Skip to content

Adjustment to get the compiler to finish the jitting#2

Merged
jtschuster merged 1 commit intojtschuster:AsyncR2Rfrom
davidwrighton:AsyncR2RTweaks
Oct 23, 2025
Merged

Adjustment to get the compiler to finish the jitting#2
jtschuster merged 1 commit intojtschuster:AsyncR2Rfrom
davidwrighton:AsyncR2RTweaks

Conversation

@davidwrighton
Copy link
Copy Markdown

Notably, I've tweak the cross module inlining feature to enable compilation of the tokens needed in these thunks. The tweak enables using these particular apis without enabling the cross module switch. It successfully finishes jitting a method.

  • This does not ... handle the dependency graph issues
  • I've noticed that we seem to make the thunks as temporary things, and assume they don't need process long lifetime. This is not likely to true, as they are actual useable symbols, so that needs to be fixed so that the various thunk methoddescs aren't treated as transient. (Anything which is a symbol or goes in the dependency graph needs to be non-transient, and able to be looked up later. Newing up a new instance is not a valid thing to do.)
  • We seem to have a concept of thunks and whatnot in the MethodDescs... I'm seeing us compile the original method... with the IL for a thunk, and then I'm seeing something try to add to the dependency graph a thunk which is supposed to have the calling convention of the method we just compiled, but is MethodDesc of type TaskReturnAsyncMethodDescThunk or something.

Notably, I've tweak the cross module inlining feature to enable compilation of the tokens needed in these thunks. The tweak enables using these particular apis without enabling the cross module switch. It successfully finishes jitting a method.

- This does not ... handle the dependency graph issues
- I've noticed that we seem to make the thunks as temporary things, and assume they don't need process long lifetime. This is not likely to true, as they are actual useable symbols, so that needs to be fixed so that the various thunk methoddescs aren't treated as transient. (Anything which is a symbol or goes in the dependency graph needs to be non-transient, and able to be looked up later. Newing up a new instance is not a valid thing to do.)
- We seem to have a concept of thunks and whatnot in the MethodDescs... I'm seeing us compile the original method... with the IL for a thunk, and then I'm seeing something try to add to the dependency graph a thunk which is supposed to have the calling convention of the method we just compiled, but is MethodDesc of type TaskReturnAsyncMethodDescThunk or something.
@jtschuster jtschuster merged commit 4802a7f into jtschuster:AsyncR2R Oct 23, 2025
jtschuster added a commit that referenced this pull request Feb 3, 2026
Implements change #2 from async methods R2R breakdown.

Adds helper 0x27 for throwing exact exceptions in R2R images.
jtschuster pushed a commit that referenced this pull request Mar 25, 2026
…otnet#124642)

## Summary

Fixes dotnet#123621

When a constant-folded operand appears **after** a non-constant operand
in a short-circuit `&&` expression (e.g., `v == 2 && Environment.NewLine
!= "\r\n"`), callee inlining can leave dead local stores in the return
block. The `isReturnBool` lambda in `fgFoldCondToReturnBlock` required
`hasSingleStmt()`, which caused the optimization to bail out when these
dead stores were present, resulting in suboptimal branching codegen.

### Changes

- **`src/coreclr/jit/optimizebools.cpp`**: Relax the `hasSingleStmt()`
constraint in `isReturnBool` to allow preceding statements as long as
they have no globally visible side effects
(`GTF_GLOBALLY_VISIBLE_SIDE_EFFECTS`). This enables
`fgFoldCondToReturnBlock` to fold the conditional into a branchless
return even when dead local stores from inlining remain in the block.

### Before (ARM64, `Inline_After`)
```asm
      cmp     w0, #2
      bne     G_M4495_IG04
      mov     w0, #1
      ret
G_M4495_IG04:
      mov     w0, #0
      ret
```

### After (ARM64, `Inline_After`)
```asm
      cmp     w0, #2
      cset    x0, eq
      ret
```

## Test plan

- [x] Added regression test `Runtime_123621` covering the original issue
pattern
- [x] Verified `Hoisted`, `Inline_Before`, and `Inline_After` all
produce identical branchless codegen (`cset` on ARM64)
- [x] Verified existing `DevDiv_168744` regression test still passes
- [x] Verified side-effect-ful blocks are correctly excluded from the
optimization
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants