Skip to content

Conversation

AndyAyersMS
Copy link
Member

If a JTRUE comparison only involves part of a local value we cannot make assertions about the local as a whole.

Fixes #111352.

If a JTRUE comparison only involves part of a local value we cannot make assertions
about the local as a whole.

Fixes dotnet#111352.
@Copilot Copilot AI review requested due to automatic review settings February 12, 2025 23:09
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 12, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 3 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • src/coreclr/jit/assertionprop.cpp: Language not supported
  • src/tests/JIT/Regression/JitBlue/Runtime_111352/Runtime_111352.csproj: Language not supported

@AndyAyersMS
Copy link
Member Author

@EgorBo PTAL
cc @dotnet/jit-contrib

Surprising (though small) number of diffs though many of those look like bool cases where the bits we don't compare happen to be zero, so we were getting lucky.

This only affects local AP an only in the (relatively new) cross-block mode introduced in #94741.

This was reported as a .NET 9 issue so I will propose a backport.

@jakobbotsch
Copy link
Member

Surprising (though small) number of diffs though many of those look like bool cases where the bits we don't compare happen to be zero, so we were getting lucky.

Can you share some of those cases? I think it should be ok to scope the check here to TYP_INT uses of TYP_LONG LclVarDsc. The small cases come with normalization guarantees.

@AndyAyersMS
Copy link
Member Author

here is one of the simpler ones

29948.base.txt
29948.diff.txt

@jakobbotsch
Copy link
Member

The base there looks ok to me. V04 is normalize-on-store, so its upper 24 bits are always going to be zero

@AndyAyersMS
Copy link
Member Author

So you think just the TYP_LONG locals are at risk?

@AndyAyersMS
Copy link
Member Author

Seems plausible because this sort of pattern must be rare or we'd have found this long ago.

@jakobbotsch
Copy link
Member

jakobbotsch commented Feb 13, 2025

So you think just the TYP_LONG locals are at risk?

I think so, yes. We allow accessing lower 32 bits of TYP_LONG locals with TYP_INT GenTreeLclVar, but the same thing is not allowed for smaller types (a small typed GenTreeLclVar really does mean a 32 bit int with some form of normalization in the upper bits).
There was a somewhat recent discussion about getting rid of this former case with the Samsung folks, at least until lowering... Seems like here we have some more motivation. optNarrowTree is one of the places that creates it today.

@AndyAyersMS
Copy link
Member Author

Ok, did that. Now no diffs locally.

@AndyAyersMS AndyAyersMS merged commit a5f99c6 into dotnet:main Feb 13, 2025
117 checks passed
@AndyAyersMS
Copy link
Member Author

/backport to release/9.0

Copy link
Contributor

Started backporting to release/9.0: https://github.com/dotnet/runtime/actions/runs/13317332242

@AndyAyersMS
Copy link
Member Author

/backport to release/9.0-staging

Copy link
Contributor

Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/13318074756

@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compilation problem in .Net 9.0
3 participants