Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Make wasm 'V' signature raising deterministic and add a regression test by tannergooding · Pull Request #131429 · dotnet/runtime · GitHub
Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Make wasm 'V' signature raising deterministic and add a regression test by tannergooding · Pull Request #131429 · dotnet/runtime · GitHub
Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Make wasm 'V' signature raising deterministic and add a regression test by tannergooding · Pull Request #131429 · dotnet/runtime · GitHub
Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Make wasm 'V' signature raising deterministic and add a regression test by tannergooding · Pull Request #131429 · dotnet/runtime · GitHub
Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Make wasm 'V' signature raising deterministic and add a regression test by tannergooding · Pull Request #131429 · dotnet/runtime · GitHub
Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Make wasm 'V' signature raising deterministic and add a regression test by tannergooding · Pull Request #131429 · dotnet/runtime · GitHub
Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Make wasm 'V' signature raising deterministic and add a regression test by tannergooding · Pull Request #131429 · dotnet/runtime · GitHub
Skip to content

Make wasm 'V' signature raising deterministic and add a regression test - #131429

Merged
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism
Jul 27, 2026
Merged

Make wasm 'V' signature raising deterministic and add a regression test#131429
tannergooding merged 1 commit into
dotnet:mainfrom
tannergooding:tannergooding-wasm-v128-raising-determinism

Conversation

@tannergooding

@tannergoodingtannergooding commented Jul 27, 2026

Copy link
Copy Markdown
Member

Fixes#131339 -- though not in the way the issue proposed; see the note at the bottom.

RaiseSignature resolved the 'V' (v128) signature char to CompilerTypeSystemContext.CachedV128Type, i.e. literally whichever v128 type lowering happened to encounter first, written racily from parallel compilation. 'V' is fully determined by the wasm ABI (16 bytes, 16-byte aligned), so raising now resolves a fixed canonical Vector128<byte> instead. Any v128 type round-trips 'V' identically, so RaiseSignature's own round-trip assert still holds, and the raised signature no longer depends on compilation order.

The 16-byte alignment Debug.Assert that guarded this invariant moved from CacheV128Type into IsWasmV128Type. CacheV128Type only ever saw the first cached type, where-as IsWasmV128Type has three call sites and so covers every v128 type.

This also drops the ?? throw new InvalidOperationException(...) on the 'V' path -- that failure mode is simply gone, since raising no longer needs lowering to have run first.


ILCompiler.ReadyToRun.Tests gains WasmArgumentLayoutTests: 20 cases driving crossgen2's type system and GCRefMapBuilder.BuildArgIterator directly, so they need neither a wasm JIT nor a runtime to execute against.

  • WasmV128TypesAre16ByteAligned -- 12 cases over Vector128<T>/Vector<T> x 6 element types.
  • WasmV128ArgumentsStartOn16ByteBoundaries -- static void M(long, TVector, ref int) must lay out as [0, 16, 32].
  • OtherSimdWidthsAreNotV128 -- Vector64/256/512<T> must fall back to the generic struct ABI.
  • RaisingV128SignatureIsIndependentOfLoweringOrder -- lowers a different vector type first, then asserts the raised 'V' is unchanged and that its offsets match the original signature's.

These also cover the Vector<T> alignment fix from #131328, and I verified they bite: with that fix reverted, 7 cases fail -- the six Vector<T> alignment cases report 8 instead of 16, and WasmV128ArgumentsStartOn16ByteBoundaries for Vector<T> reports [0, 8, 24] instead of [0, 16, 32].


On #131339 itself. The issue asks for a test asserting Vector128<T> is 16-byte aligned, on the premise that this is what #131328 fixed. That is not correct -- Vector128<T> gets alignment 16 from VectorFieldLayoutAlgorithm on every architecture except ARM and was untouched by that fix. I confirmed empirically that the proposed test passes unchanged against a compiler with #131328 reverted, so it would have been a no-op. The actual regression was System.Numerics.Vector<T>, which kept the 8-byte alignment from its metadata layout. The tests here cover both.

The issue also says the test was deferred for lack of a CI-runnable home. That premise is stale: #130866 added the WasmSimdModule wasm crossgen2 suite to ILCompiler.ReadyToRun.Tests three days before the issue was filed, and that is where these tests live.


Also hoisted the System.Private.CoreLib.dll resolution out of R2RTestRunner into TestPaths.SystemPrivateCoreLibPath so the new tests share the existing runtime-pack to CoreCLR-artifacts fallback. Without it they would hard-fail in partial builds that skip libs.pretest, e.g. a bare clr.toolstests.

Out of scope, to be filed separately: CacheStructBySize/GetCachedStructOfSize has the same first-wins-cache shape for the 'S<N>' encoding, and there it is a live miscompile rather than a latent hazard. Guid and Int128 are both 16 bytes and both encode S16, but their clamped alignments are 8 and 16, so the shared thunk signature vlS16ip gets two different frame layouts. Fixing that means changing the on-disk encoding across crossgen2, src/coreclr/vm/wasm/helpers.cpp, WasmAppBuilder, and the R2R format doc, so it does not belong here.

Local test run: ILCompiler.ReadyToRun.Tests is 58 total / 1 failed. The failure is CompositeManifestAssemblyMvidsArePaddedWhenPdbPresent, an ArgumentNullException out of the native PDB COM writer in my Debug layout. Confirmed pre-existing -- it reproduces identically with this branch's changes stashed.

CC. @lewing@adamperlin -- ready for review.

Note

This PR description was drafted by Copilot.

RaiseSignature resolved 'V' to whichever v128 type lowering happened to see
first, cached racily from parallel compilation. 'V' is fully determined by the
ABI, so resolve a fixed Vector128<byte> instead, and move the 16-byte alignment
assert into IsWasmV128Type so it covers every v128 type rather than just the
first one cached.
The test also covers the Vector<T> alignment fix from dotnet#131328; issue dotnet#131339
proposed testing Vector128<T>, which was never under-aligned.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings July 27, 2026 17:28
@github-actionsgithub-actionsBot added the area-crossgen2-coreclr only use for closed issues label Jul 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes wasm signature raising for the 'V' (v128) encoding deterministic by resolving it to a fixed canonical type (Vector128<byte>) rather than relying on whichever v128 type happened to be cached first during lowering. It also adds targeted regression tests in ILCompiler.ReadyToRun.Tests to validate v128 alignment and argument layout invariants for the wasm ABI, and refactors test infrastructure to share a consistent System.Private.CoreLib.dll resolution path.

Changes:

  • Make RaiseSignature map 'V' to CompilerTypeSystemContext.WasmV128Type (canonical Vector128<byte>) and move the 16-byte alignment invariant assertion into IsWasmV128Type.
  • Add WasmArgumentLayoutTests covering v128 type alignment, argument offsets, non-v128 SIMD widths, and raising independence from lowering order.
  • Hoist System.Private.CoreLib.dll path resolution into TestPaths.SystemPrivateCoreLibPath and reuse it from R2RTestRunner and the new tests.
Show a summary per file
FileDescription
src/coreclr/tools/Common/JitInterface/WasmLowering.csRemoves order-dependent v128 caching during lowering, asserts v128 alignment via IsWasmV128Type, and raises 'V' via a canonical type.
src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.csReplaces the “first-seen” v128 cache with a lazy canonical Vector128<byte> (WasmV128Type).
src/coreclr/tools/aot/ILCompiler.ReadyToRun/ILCompiler.ReadyToRun.csprojAdds InternalsVisibleTo to allow the test project to access internal crossgen2 APIs.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/ILCompiler.ReadyToRun.Tests.csprojAdds an aliased reference to ILCompiler.ReadyToRun for internal API access without namespace/type collisions.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.csNew regression tests validating wasm v128 alignment and argument layout behavior, including raising determinism.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/TestPaths.csAdds SystemPrivateCoreLibPath with runtime-pack → CoreCLR-artifacts fallback.
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/TestCasesRunner/R2RTestRunner.csSwitches SPCL resolution to the shared TestPaths.SystemPrivateCoreLibPath.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 0

@davidwrightondavidwrighton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the compiler changes, but please get Jackson to take a look at the test changes.

@tannergooding
tannergooding enabled auto-merge (squash) July 27, 2026 19:00
@tannergooding
tannergooding merged commit 79a5a09 into dotnet:mainJul 27, 2026
114 checks passed
@tannergooding
tannergooding deleted the tannergooding-wasm-v128-raising-determinism branch July 27, 2026 20:40
@dotnet-milestone-botdotnet-milestone-botBot added this to the 11.0-rc1 milestone Jul 28, 2026
lewing added a commit that referenced this pull request Jul 29, 2026
`main` does not currently build. The `linux-x64 checked CLR_Tools_Tests`
leg fails in `Build product`:
```
src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs(166,45): error CS7036: There is no argument given that corresponds to the required parameter 'instructionSetSupport' of 'ReadyToRunCompilerContext.ReadyToRunCompilerContext(TargetDetails, SharedGenericsMode, bool, bool, InstructionSetSupport, CompilerTypeSystemContext)'
```
## Cause
A semantic merge conflict between two PRs that landed ~18 hours apart,
so neither one's CI saw the other:
- #131429 (merged 2026-07-27T20:14Z) added `WasmArgumentLayoutTests.cs`,
whose `CreateWasmContext` constructs `ReadyToRunCompilerContext` with 5
arguments.
- #130622 (merged 2026-07-28T14:04Z) added a `bool
targetAllowsRuntimeCodeGeneration` parameter to that constructor, making
it 6.
## Fix
Pass the missing argument. `false` is the value crossgen2 itself
computes for this target: `Program.GetTargetAllowsRuntimeCodeGeneration`
returns `false` when the OS is `Browser`/`Wasi`/Apple-mobile or the
architecture is `Wasm32`, and this test context mirrors `--targetarch
wasm --targetos browser` (as its own doc comment states). The flag
governs whether `Vector<T>` stays optimistic and whether
explicitly-unsupported ISA markers are emitted — on wasm there is no
runtime codegen to fall back to.
## Validation
- `./build.sh clr+libs -rc Checked` succeeds, and
`ILCompiler.ReadyToRun.Tests.csproj` then compiles clean — the exact
compile that fails in CI.
- All 20 `WasmArgumentLayoutTests` cases pass, so the regression
coverage #131429 added still tests what it was written to test.
Other tests in that assembly fail on my macOS-arm64 box for an unrelated
local reason (`DllNotFoundException` for `clrjit_universal_arm64_arm64`
/ `clrjit_unix_x64_arm64` — `build.sh clr+libs` publishes only the wasm
cross-jit into the crossgen2 output directory on this host). Those
suites invoke crossgen2 as a subprocess and do not touch the changed
file.
Fixes#131504
> [!NOTE]
> This change was developed with GitHub Copilot and reviewed by me
before submitting.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: eb624f92-0ff3-435c-8963-5b203384fbed
@pavelsavarapavelsavara added the arch-wasm WebAssembly architecture label Jul 29, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

@github-actionsgithub-actionsBot locked and limited conversation to collaborators Aug 28, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmWebAssembly architecturearea-crossgen2-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add crossgen ArgIterator unit test for wasm Vector128 argument 16-byte alignment

7 participants

@tannergooding@lewing@adamperlin@davidwrighton@jtschuster@pavelsavara