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
ghost
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 |
| { | ||
| // 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