Repository navigation
JIT: model the signed Log2 fallback as throwing - #135485
Merged
Merged
Conversation
PR description: <!-- --> Signed `int.Log2`/`long.Log2` are imported as `x < 0 ? INTRINSIC(Log2, x) : lzcnt-based result`, where the intrinsic fallback is later rewritten into the throwing managed call. The fallback node only had a hand-set `GTF_CALL`; `OperExceptions` reported no exceptions for it and side effect checks only treat real calls as calls. So when the result was unused, the dead store containing the fallback was removed along with the `ArgumentOutOfRangeException`. Report the fallback intrinsic as possibly throwing (`OperExceptions`, `GTF_EXCEPT`), and give it an opaque exception set in value numbering, as the `NamedIntrinsic` guidance for throwing `GT_INTRINSIC`s requires. Keeping the fallback exposed a latent bug in `fgExpandQmarkStmt`: for qmarks with only a then arm, the condition is reversed but the then/else likelihoods were assigned to the opposite edges, producing inconsistent profile data when the then arm is rare. Assign them to the correct edges. No asm diffs in libraries.pmi (windows-x64). Add a single-file regression for used and unused signed Log2 results. Fixes dotnet#133965 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 15ca58a5-0ca4-4d38-8775-43e1c7aa8150
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Regression coverage omits nint.Log2, which uses the same signed fallback path.
1 open finding
What changed in this PR
Fixes signed Log2 exception handling when the result is discarded and corrects associated qmark profile likelihoods.
Changes:
- Marks signed
Log2fallback intrinsics as throwing. - Preserves exceptions during value numbering.
- Adds regression coverage and corrects qmark edge likelihoods.
| File | Description |
|---|---|
src/coreclr/jit/gentree.cpp |
Reports Log2 fallback exceptions. |
src/coreclr/jit/importercalls.cpp |
Adds GTF_EXCEPT to the fallback. |
src/coreclr/jit/morph.cpp |
Corrects qmark edge likelihood assignment. |
src/coreclr/jit/valuenum.cpp |
Models an opaque exception set. |
src/tests/JIT/Regression_ro_2/Runtime_133965.cs |
Tests discarded signed Log2 results. |
🧠 Review effort: Balanced
Added TestNative method to test Log2 for nint type. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Member
Author
|
PTAL @dotnet/jit-contrib |
Open
3 tasks
JulieLeeMSFT
approved these changes
Oct 9, 2026
JulieLeeMSFT
left a comment
JulieLeeMSFT
left a comment
Member
There was a problem hiding this comment.
LGTM. The signed log2 fallback seems correct.
EgorBo
enabled auto-merge (squash)
October 10, 2026 16:45
Member
Author
|
/ba-g timeout |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed
int.Log2/long.Log2are imported asx < 0 ? INTRINSIC(Log2, x) : lzcnt-based result, where the intrinsic fallback is later rewritten into the throwing managed call. The fallback node only had a hand-setGTF_CALL;OperExceptionsreported no exceptions for it and side effect checks only treat real calls as calls. So when the result was unused, the dead store containing the fallback was removed along with theArgumentOutOfRangeException.Report the fallback intrinsic as possibly throwing (
OperExceptions,GTF_EXCEPT), and give it an opaque exception set in value numbering, as theNamedIntrinsicguidance for throwingGT_INTRINSICs requires.Keeping the fallback exposed a latent bug in
fgExpandQmarkStmt: for qmarks with only a then arm, the condition is reversed but the then/else likelihoods were assigned to the opposite edges, producing inconsistent profile data when the then arm is rare. Assign them to the correct edges.No asm diffs
Fixes #133965