JIT: reject variable-count XOR rotation recognition - #133867
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 this area: @JulieLeeMSFT, @jakobbotsch |
Co-authored-by: EgorBo <523221+EgorBo@users.noreply.github.com>
|
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. |
There was a problem hiding this comment.
🟢 Approval recommended
The fix and regression coverage address the reported JIT miscompile.
Pull request overview
Fixes a RyuJIT miscompile by rejecting variable-count XOR patterns as rotations.
Changes:
- Restricts XOR rotation recognition to valid constant-count patterns.
- Adds regression coverage for widths, directions, boundaries, and constant rotations.
File summaries
| File | Description |
|---|---|
src/coreclr/jit/morph.cpp |
Prevents invalid variable-count XOR-to-rotation transformations. |
src/tests/JIT/CodeGenBringUpTests/Rotate.cs |
Adds regression tests for XOR rotation edge cases. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
|
PTAL @dhartglassMSFT |
tannergooding
left a comment
There was a problem hiding this comment.
LGTM. That being said, the rotation recognition logic here is "very complex" for something where ideally most users would just use int.RotateLeft or similar.
We also support "basically any pattern" that resembles a rotation, rather than pushing users towards a single standard pattern.
I think it'd be worth simplifying this in .NET 12 and telling users to fix their code or use the dedicated helpers. We really don't need this to handle every scenario possible or be this complex.
When the shift count is a multiple of the operand width, both shifts yield
x: XOR must return zero, whereas a rotation returnsx.