Implement ReadyToRunGenericHelperNode for WASM - #134078
jtschuster with Copilot wants to merge 4 commits into
Conversation
|
Azure Pipelines: 16 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
|
Tagging subscribers to this area: @agocke, @dotnet/ilc-contrib |
Co-authored-by: jtschuster <36744439+jtschuster@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate stack/dictionary-pointer issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds WASM ReadyToRun generic helper emission for dictionary, static-base, thread-static, delegate, type-context, and handle lookups.
Changes:
- Defines WASM helper signatures and lookup emission.
- Handles invalid dictionary entries via
ThrowUnavailableType. - Supports generic static and delegate helpers.
File summaries
| File | Description |
|---|---|
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunGenericHelperNode.cs |
Adds WASM generic helper code generation. |
Review details
Suppressed comments (5)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunGenericHelperNode.cs:125
- ❌ The
Teepreserves the index-cell value on the stack while the code also loads the type-manager pointer and thread-static index from it. Together with the initialLocal.Get(0), the call stack becomes[SP, indexCell, manager, index], leaving an extra operand; the corresponding non-generic WASM helper passes only[SP, manager, index]. Consume the lookup result withLocal.Setbefore reloading it for the two loads.
[!NOTE] This review comment was generated by GitHub Copilot.
expressions.Add(Local.Tee(resultLocalIndex));
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunGenericHelperNode.cs:199
- The generic context is not at these local indices: the non-delegate signature has one parameter (so its context is local 0), and the delegate signature has three parameters (so its context is local 2).
Local.Get(1)/Local.Get(3)therefore read the first uninitialized local, causing every lookup to use an invalid dictionary pointer. Load index 0 for regular helpers and index 2 forDelegateCtor, matching the argument layout used by the other target implementations.
protected virtual void EmitLoadGenericContext(
NodeFactory factory,
List<WasmExpr> expressions,
bool relocsOnly)
{
expressions.Add(Local.Get(Id == ReadyToRunHelperId.DelegateCtor ? 3 : 1));
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunGenericHelperNode.cs:154
- For
DelegateCtor, the third parameter is the generic context and is overwritten byEmitDictionaryLookupwith the target method before invoking the constructor. PassingLocal.Get(2)here also passes the unmodified generic context as a third constructor argument, shifting the lookup result and optional thunk to the wrong positions (the x64 implementation only forwards Arg0/Arg1 before replacing Arg2). Remove this expression so the constructor receives its expected arguments.
expressions.Add(Local.Get(0));
expressions.Add(Local.Get(1));
expressions.Add(Local.Get(2));
EmitDictionaryLookup(factory, expressions, contextLocalIndex, resultLocalIndex, LookupSignature, relocsOnly);
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunGenericHelperNode.cs:115
- In the lazy-cctor GC-static path, this subtraction leaves the adjusted non-GC static-base pointer on the operand stack, but the next
EmitDictionaryLookupreadscontextLocalIndex, which still contains the original dictionary pointer. Store the adjusted pointer back tocontextLocalIndexbefore the second lookup; otherwise the GC lookup is performed against the wrong dictionary.
GenericLookupResult nonGcRegionLookup =
factory.GenericLookup.TypeNonGCStaticBase((MetadataType)Target);
EmitDictionaryLookup(factory, expressions, contextLocalIndex, resultLocalIndex, nonGcRegionLookup, relocsOnly);
expressions.Add(I32.Const(NonGCStaticsNode.GetClassConstructorContextSize(factory.Target)));
expressions.Add(I32.Sub);
EmitDictionaryLookup(factory, expressions, contextLocalIndex, resultLocalIndex, LookupSignature, relocsOnly);
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_Wasm/WasmReadyToRunGenericHelperNode.cs:69
- This adds all of the WASM generic-helper code paths without a WASM regression test. The existing generic lookup R2R test is explicitly excluded on WASM (
R2RTestSuites.cs:2168), and the current WASM tests only compile non-generic smoke cases, so failures in these stack layouts, helper signatures, or invalid-slot handling can regress unnoticed. Add a WASM-targeted R2R test that forces representative generic dictionary/type lookups (including a static or delegate helper) and validates that crossgen2 emits the module successfully.
[!NOTE] This review comment was generated by GitHub Copilot.
protected sealed override void EmitCode(NodeFactory factory, ref WasmEmitter encoder, bool relocsOnly)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The lazy non-GC static-base path generates an invalid operand stack and class-constructor context.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Resolved since last review (1)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new code-generation paths lack targeted WASM regression coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
Resolved since last review (1)
|
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. |


WASM compilation could not emit ReadyToRun generic helper stubs because their architecture-specific implementation was missing.
Changes
ThrowUnavailableType.Note
This description was generated by GitHub Copilot.