Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix morph negation transforms #55145

Merged
merged 5 commits into from
Jul 5, 2021

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jul 4, 2021

The short version of the bug is that the transforms in question did not account for persistent side effects, which could lead to silent bad codegen. Also, the GTF_EXCEPT guards were unnecessary for transforms that did not reorder operands.

This change leads to one diff in the benchmarks, a regression of 2 bytes, caused by the fact that gtCanSwapOrder does not understand that we can swap when both trees have GTF_GLOB_REF set, but no other effects on them:

-               [000078] ----G+------              \--*  SUB       int
-               [000077] ----G+------                 +--*  LCL_VAR   int   (AX) V11 loc6
-               [000075] ----G+------                 \--*  LCL_VAR   int   (AX) V08 loc3
+               [000078] ----G+------              \--*  ADD       int
+               [000076] ----G+------                 +--*  NEG       int
+               [000075] ----G+------                 |  \--*  LCL_VAR   int   (AX) V08 loc3
+               [000077] ----G+------                 \--*  LCL_VAR   int   (AX) V11 loc6

Fixes #55140.

Note: I am aware that gtCanSwapOrder is not without its problems as well, but it still seems decidedly better to use it instead of bespoke guards.

Also relax the condition for SUB(a, (NEG(b)) => ADD(a, b).

No diffs.
Also relax the condition for ADD(a, (NEG(b)) => SUB(a, b).
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 4, 2021
The plan was to move the test to Pri1, but I suppose that's not really necessary given it's rather modest size.

Co-authored-by: Egor Bogatov <[email protected]>
A copy & paste mistake.
@jakobbotsch jakobbotsch merged commit 5065d3b into dotnet:main Jul 5, 2021
@jakobbotsch
Copy link
Member

Thanks for fixing this!

@SingleAccretion SingleAccretion deleted the Fix-Morph-Neg-Transforms branch July 5, 2021 16:46
@ghost ghost locked as resolved and limited conversation to collaborators Aug 4, 2021
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.

JIT incorrectly reorders method call and static field load
3 participants