-
Notifications
You must be signed in to change notification settings - Fork 5.3k
JIT: remove anticipated demand for throw helpers #123781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Historically morph would signal to the backend that a throw helper might be needed from a certain block by calling `fgAddCodeRef`. This scheme is less viable now that we have targets like Wasm where null checks must be explicit. Modify the JIT so that throw helper demand and throw helper block/call creation is all done during the stack level setting phase, so there is no need to anticipate if throw helpers will be needed in advance. Also, always minimize the set of common throw helpers needed if not generating debuggable code (where throw helper calls are "in line").
There was a problem hiding this comment.
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 moves throw-helper “demand” and throw-helper block/call creation out of morph and into the stack-level setting phase, avoiding the need to anticipate helper usage (notably for targets like Wasm that require explicit null checks).
Changes:
- Removed morph-time “anticipation” of throw helpers by deleting
fgAddCodeRefcalls at morph sites. - Reworked throw-helper infrastructure to create
AddCodeDscentries and blocks on demand during stack-level setting (fgCreateAddCodeDsc,fgCreateThrowHelperBlock, updatedfgFindExcptnTarget). - Ensured throw helper code insertion and unused-helper pruning happens during
StackLevelSetter.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/stacklevelsetter.cpp | Centralizes throw-helper discovery and creation during stack-level setting; updates helper usage tracking/removal. |
| src/coreclr/jit/morph.cpp | Removes morph-time throw-helper “demand” signaling (fgAddCodeRef calls). |
| src/coreclr/jit/lower.cpp | Updates comment to reflect that throw-helper calls may occur (not necessarily known upfront). |
| src/coreclr/jit/flowgraph.cpp | Replaces fgAddCodeRef with descriptor/block creation APIs and adjusts helper-block creation flow. |
| src/coreclr/jit/compiler.h | Updates declarations for new throw-helper APIs and exposes fgRngChkThrowAdded. |
| src/coreclr/jit/compiler.cpp | Removes the dedicated “create throw helpers” phase from the pipeline. |
| src/coreclr/jit/codegencommon.cpp | Updates comment wording around sharing throw helper blocks. |
|
FYI @dotnet/jit-contrib Mainly motivated by Wasm, but reduces coupling overall. There is more cleanup that could be done, since we should no longer need to update ACDs when adding or removing EH regions, but I've left that code in place for now. It will be bypassed at runtime since the ACD map will be empty when phases that modify EH run. A modest number of more or less neutral diffs, some from throw helper blocks being reordered, and some places where we now insert an explicit runtime/src/coreclr/jit/lower.cpp Line 3335 in 062177e
fired when we created throw helper blocks before lower, but now we defer creation until after, and so in more cases now there's just one block (and in the example I look at the throw helper block ended up unused and was removed). And a few diffs where the allocator behaves differently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
Nice to see this hack go away from morph. Any idea why the TP is up to +0.86% for MinOpts? |
If we are going to use throw helper blocks (which we do unless we are asked for debugggable codegen) we have to walk the IR now even in minopts to figure out where we need throw helpers. So this is roughly the cost of that IR walk. Previously we relied on the upstream demands to describe all the helpers we would need, so didn't need to do this walk. |
We might be able to mitigate this cost somewhat if we can determine that we won't need any throw helpers in some upstream phase (eg there are no oper may throw GTF_EXCEPT nodes). I would guess a decent fraction of methods don't need any helpers. But it might end up being fragile. |
Yeah, kind of similar to insert-gc-polls phase, which can rely on a BBF_ flag for Tier0. Although, for that phase it's not catastrophic if it misses something |
|
The SPMI asmdiff failure is in a method with runtime async, we blow up trying to display a This PR has modified block layout slightly. With just disasm it blows up later, during emit display @jakobbotsch does this sound at all familiar? |
|
The throughput optimization is relatively new (#112561). I personally think it's ok to trade it off for the robustness we get here. |
Hmm, no, I haven't seen this before. |
|
I think it's a dump/disasm only issue. This bit of code doesn't look for Seems like we can just generalize it. runtime/src/coreclr/jit/emitarm.cpp Lines 7284 to 7308 in 918254d
|
Not so simple. Maybe we need a new |
|
Opened #123813 for this arm32 failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Fix range mechanics Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/stacklevelsetter.cpp:190
checkForHelpersis now always initialized to true, making the surrounding optimization-related comment and the subsequentcheckForHelpers |= !framePointerRequiredlogic redundant. Consider simplifying this block (or updating the comment) so it reflects the new always-check behavior.
// When optimizing we want to know what throw helpers might be used
// so we can remove the ones that aren't needed.
//
// If we're not optimizing then the helper requests made in
// morph are likely still accurate, so we don't bother checking
// if helpers are indeed used.
//
bool checkForHelpers = true;
#if !FEATURE_FIXED_OUT_ARGS
// Even if not optimizing, if we have a moving SP frame, a shared helper may
// be reached with mixed stack depths and so force this method to use
// a frame pointer. Once we see that the method will need a frame
// pointer we no longer need to check for this case.
//
checkForHelpers |= !framePointerRequired;
#endif
if (checkForHelpers)
{
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/codegenwasm.cpp:1206
fgGetExcptnTargetno longer returnsnullptr(it creates anAddCodeDscon demand), so theif (add == nullptr)/assert(add != nullptr)block is now dead code. More importantly, if a null-check demand was missed earlier, this code will still proceed and may end up dereferencing a nulladd->acdDstBlkin non-assert builds. Consider replacing the null check with a check for the expected preconditions (e.g.,add->acdUsedand/oradd->acdDstBlk != nullptr) and keep theNYI_WASM("Missing null check demand")behavior for that case.
Compiler::AddCodeDsc* const add = compiler->fgGetExcptnTarget(SCK_NULL_CHECK, compiler->compCurBB);
if (add == nullptr)
{
NYI_WASM("Missing null check demand");
}
assert(add != nullptr);
assert(add->acdUsed);
GetEmitter()->emitIns_I(INS_I_const, EA_PTRSIZE, compiler->compMaxUncheckedOffsetForNullObject);
GetEmitter()->emitIns(INS_I_le_u);
inst_JMP(EJ_jmpif, add->acdDstBlk);
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/codegenwasm.cpp:1199
fgGetExcptnTargetno longer returnsnullptr(it creates anAddCodeDscon-demand), so thisif (add == nullptr)/NYI_WASMbranch is now dead code. More importantly, the thing that can actually be null here isadd->acdDstBlkwhen the helper block wasn’t created; consider removing this null check and instead validatingadd->acdDstBlk(ideally with anoway_assert) before using it ininst_JMP.
Compiler::AddCodeDsc* const add = compiler->fgGetExcptnTarget(SCK_NULL_CHECK, compiler->compCurBB);
if (add == nullptr)
{
NYI_WASM("Missing null check demand");
adamperlin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pieces I understand look good to me. It looks like that outstanding copilot suggestion about the comment may still be relevant?
Co-authored-by: Copilot <[email protected]>
Thanks. I went with copilot's suggestion here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/codegenwasm.cpp:1203
- The
if (add == nullptr)/NYI_WASM("Missing null check demand")path is now effectively dead: fgGetExcptnTarget currently creates a new AddCodeDsc when none exists, soaddwon't be null here. If the intent is to require that stacklevelsetter has already created the null-check helper block, consider removing this null check and instead asserting thatadd->acdDstBlkis non-null (or adjust fgGetExcptnTarget to return nullptr when no entry exists and createIfNeeded is false).
Compiler::AddCodeDsc* const add = compiler->fgGetExcptnTarget(SCK_NULL_CHECK, compiler->compCurBB);
if (add == nullptr)
{
NYI_WASM("Missing null check demand");
}
assert(add != nullptr);
assert(add->acdUsed);
|
SPMI failure is #123813 |
|
For the other failures: looks like codegen for some NAOT HW intrinsic feature tests must have changed. The validation does an inspection of the code stream: runtime/src/tests/nativeaot/SmokeTests/HardwareIntrinsics/Program.cs Lines 616 to 636 in 087c690
|
## Description
ARM32 JIT dump/disasm asserts when encountering async resume info
references. The `IF_T2_N2` instruction format handles both block address
tables and async resume info, but the dump logic only expected the
former.
## Changes
- **Remove assert in `emitarm.cpp:7308`**: Was failing when `jdsc` is
null (async resume info case)
- **Add null check before table dump**: Skip inline block address table
dump when `jdsc` is null
- **Use `nullptr` instead of `NULL`**: Modern C++ style throughout the
code block
- **Add conditional printing**: Print `RWD%02zu` format for async resume
info (when `jdsc` is null) and `J_M%03u_DS%02u` format for block address
tables (when `jdsc` is not null)
- **Clarify comment**: Document when `jdsc` is null vs non-null
The fix preserves all existing dump behavior for block address tables
while allowing async resume info to display properly without assertion.
```cpp
// Before: would assert if jdsc is null and always print as DS format
assert(jdsc != NULL);
printf("... J_M%03u_DS%02u", ...);
if (id->idIns() == INS_movt) { /* dump table using jdsc */ }
// After: skip table dump when jdsc is null and use appropriate format
if (jdsc != nullptr)
printf("... J_M%03u_DS%02u", ...); // block address table
else
printf("... RWD%02zu", ...); // async resume info
if (jdsc != nullptr && id->idIns() == INS_movt) { /* dump table using jdsc */ }
```
Affects dump/disasm output only; no impact on code generation.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
>
> ----
>
> *This section details on the original issue you should resolve*
>
> <issue_title>JIT: fix arm32 dump/disasm of references to async resume
info</issue_title>
> <issue_description>See
#123781 (comment)
>
> Seemingly `IF_T2_N2` expects the data to resolve to a table of block
addresses, so if we reuse this form for references to async resume info
it doesn't display properly.
>
> I think this just impacts dumping and disasm. But not 100% sure.
> </issue_description>
>
> <agent_instructions>Fix this issue by handling gracefully the
situation where `jdsc` is null -- skip the inline table dump logic in
that case</agent_instructions>
>
> ## Comments on the Issue (you are @copilot in this section)
>
> <comments>
> </comments>
>
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
- Fixes #123813
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: jakobbotsch <[email protected]>
|
New codegen - the stack frame is bigger now because this is debuggable codegen and NAOT didn't have the starup hook we see for jitted code. Only happens for debuggable leaf NAOT methods so the actual stack size increase is not a big deal. X64Avx2_Program__X86BaseIsSupported:
00000001401596E0: 55 push rbp
00000001401596E1: 57 push rdi
00000001401596E2: 48 83 EC 28 sub rsp,28h
00000001401596E6: 48 8D 6C 24 30 lea rbp,[rsp+30h]
00000001401596EB: C7 45 F4 01 00 00 mov dword ptr [rbp-0Ch],1
00
00000001401596F2: 8B 45 F4 mov eax,dword ptr [rbp-0Ch]
00000001401596F5: 0F B6 C0 movzx eax,al
00000001401596F8: 48 83 C4 28 add rsp,28h
00000001401596FC: 5F pop rdi
00000001401596FD: 5D pop rbp
00000001401596FE: C3 ret
00000001401596FF: 90 |
Historically morph would signal to the backend that a throw helper might be needed from a certain block by calling
fgAddCodeRef. This scheme is less viable now that we have targets like Wasm where null checks must be explicit.Modify the JIT so that throw helper demand and throw helper block/call creation is all done during the stack level setting phase, so there is no need to anticipate if throw helpers will be needed in advance.
Also, always minimize the set of common throw helpers needed if not generating debuggable code (where throw helper calls are "in line").