Fix side effect handling for certain TernaryLogic cases #114848
Conversation
There were a few cases of TernaryLogic with NOT/AND and NOT/OR that swapped operands without considering side effects. Add some new logic to hoist those side effects if necessary. Fixes dotnet#114324
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the handling of TernaryLogic cases where side effects were not properly preserved when swapping operands in NOT/AND and NOT/OR operations.
- Added a regression test (Runtime_114324.cs) to capture the issue.
- Updated hwintrinsicxarch.cpp to ensure that side effects are correctly hoisted during operand normalization.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_114324/Runtime_114324.cs | Added regression test for TernaryLogic side effect handling |
| src/coreclr/jit/hwintrinsicxarch.cpp | Updated operand swapping logic to correctly hoist side effects |
Files not reviewed (1)
- src/tests/JIT/Regression/JitBlue/Runtime_114324/Runtime_114324.csproj: Language not supported
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@tannergooding @dotnet/intel PTAL |
| @@ -4529,8 +4588,7 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic, | |||
| (control == static_cast<uint8_t>(~0xAA & 0xF0)) || // ~C & A | |||
| (control == static_cast<uint8_t>(~0xAA & 0xCC))) // ~C & B | |||
| { | |||
| // We're normalizing to ~B & C, so we need another swap | |||
| std::swap(*val2, *val3); | |||
| // We already normalized to ~B & C above. | |||
There was a problem hiding this comment.
nit: This first if could be merged with the assert below. We may not even need the assert at all since it's the only possible 6 permutations of NOT AND and NOT OR
There was a problem hiding this comment.
I kept the logic here stylized to be the same as the hoisted logic, for better or worse...
There were a few cases of TernaryLogic with NOT/AND and NOT/OR that swapped operands without considering side effects. Add some new logic to hoist those side effects if necessary.
Fixes #114324