[arm64] Disable `a % b = a & (b - 1);` optimization. #18206

sandreenko · 2018-05-30T21:42:47Z

Morph can't relay on Lower optimization that it can't guarantee.

In this test case we have such tree:

               [000041] ------------                   /--*  CAST      long <- int
               [000040] ------------                   |  \--*  CNS_INT   int    1
               [000042] ---X--------                /--*  UMOD      long  
			                                              / *** 
               [000024] ---X--------                |  \--*  MUL       long
			                                              \ ***

Moprh thinks that Lower will optimize it as a % b = a & (b - 1); , but then a tree is found to be a constant:

N034 (  1,  1) [000041] ------------                      /--*  CNS_INT   long   1 $182
N035 ( 36, 24) [000042] -A-X--------                   /--*  UMOD      long   $287
N032 (  1,  1) [000023] ------------                   |  /--*  CNS_INT   long   0 $180
N033 ( 34, 22) [000024] -A-X--------                   \--*  COMMA     long
                                                          \ ***

and Lower rejects optimizing a % b when both operands are const.

It cases assert during Lsra: assert(!"Shouldn't see an integer typed GT_MOD node in ARM64");

The fix is similar to #17338.

Fixes #17968.

PR #15690 can help to return this optimization back.

It was copied wrong previously.

Morph can't relay on Lower optimization that it can't guarantee.

sandreenko · 2018-05-30T21:46:33Z

@dotnet-bot test Windows_NT x64_arm64_altjit Checked r2r_jitstress1
@dotnet-bot test Windows_NT x64_arm64_altjit Checked r2r_jitstressregs1
@dotnet-bot test Windows_NT x64_arm64_altjit Checked r2r_jitstress2
@dotnet-bot test Windows_NT x64_arm64_altjit Checked jitstress2
@dotnet-bot test Windows_NT x86_arm_altjit Checked jitstress2_jitstressregs0x1000

mikedn · 2018-05-30T22:00:00Z

and Lower rejects optimizing a % b when both operands are const.

The reason why Lower doesn't do anything in this case is somewhat artificial. I expected that constant folding would handle such cases so that there's no reason to reproduce that logic in lowering.

But there's nothing stopping lowering from doing it and it's IMO its responsibility to produce IR suitable for the target CPU. In the particular case of c % 0 it could simply change it to c / 0 or whatever IR is suitable for the ARM codegen.

PR #15690 can help to return this optimization back.

That seems unrelated since MOD/UMOD are AFAIK not turned into helper calls on ARM64, they're transformed into a - b * (a / b). Ideally this transform would be delayed until lowering so that the frontend doesn't have to deal with such target specific issues. However, this transform allows CSE to eliminate a redundant a / b when the code contains both a / b and a % b.

sandreenko · 2018-05-30T22:16:25Z

That seems unrelated since MOD/UMOD are AFAIK not turned into helper calls on ARM64, they're transformed into a - b * (a / b). Ideally this transform would be delayed until lowering so that the frontend doesn't have to deal with such target specific issues.

I agree that we should decide to do an optimization and do the optimization in one place.

However, this transform allows CSE to eliminate a redundant a / b when the code contains both a / b and a % b.

We do not transform a % b to a - b * (a / b) on x86/x64, where it can have the same benefit, can we teach CSE to find such cases and transform % when it is profitable?

sandreenko · 2018-05-30T22:20:04Z

@mmitche Looks like ci is down, do you know about this problem?

mikedn · 2018-05-30T22:38:57Z

We do not transform a % b to a - b * (a / b) on x86/x64, where it can have the same benefit

Yes, this is a funny situation.

On ARM we don't have much of a choice since there is not % instruction so one way or another we have to transform.

On x86/x64 the transform is not required and if we do it for the sake of CSE we end up with an extra - and * if CSE fails. Then lowering would need to recognize a - b * (a / b) and turn it back into MOD so a single div instruction is emitted. But recognizing a - b * (a / b) isn't easy because it's not a tree.

can we teach CSE to find such cases and transform % when it is profitable?

I suppose CSE could try to look for an available a / b when it encounters an a % b. AFAIR I looked a bit at this in the past and it's problematic because it needs to figure out various things about a / b (e.g. VN, hash, index) without seeing it first. Probably doable but fairly involved and exotic.

And there's also the x86/64 specific case that if you have both a / b and a % b then you really want to generate a single a div b instruction, that's rather difficult to fit anywhere in the JIT.

sandreenko · 2018-05-31T18:00:45Z

@dotnet-bot test this please

We do not expect MOD/UMOD for integer/long types in lower for arm64.

sandreenko · 2018-06-01T19:08:54Z

@dotnet-bot test Windows_NT x64_arm64_altjit Checked r2r
@dotnet-bot test Windows_NT x64_arm64_altjit Checked r2r_jitstress2
@dotnet-bot test Windows_NT x64_arm64_altjit Checked jitstress2
@dotnet-bot test Windows_NT x86_arm_altjit Checked r2r

sandreenko · 2018-06-01T19:25:05Z

src/jit/lower.cpp

-#ifdef _TARGET_XARCH_
-    if (!varTypeIsFloating(node->TypeGet()))
-#endif // _TARGET_XARCH_
+    if (varTypeIsIntegral(node->TypeGet()))


@CarolEidt could you please take a look at this check? Do you remember why it was _TARGET_XARCH_ specific before?
LowerConstIntDivOrMod has assert((type == TYP_INT) || (type == TYP_LONG));, so looks like it is not correct to skip this check for arm/arm64.

The ifdef was probably intended to include the entire if, not just the condition. I don't think I enabled magic division for ARM64 when I wrote the code, it didn't have GT_MULHI back then. I see that now it's enabled for ARM64 but it's still not enabled for ARM.

It is not enabled for arm because we do not allow integer MOD/DIV in Lower on this platform.

So, I do not see a reason why we need this target ifdef now, it works fine for XARCH DIV/MOD and for arm64 DIV nodes (MOD are not allowed in Lower).

I don't see a reason why it should be target-specific either.

sandreenko · 2018-06-01T20:00:57Z

PTAL @dotnet/jit-contrib, @dotnet/arm64-contrib

CarolEidt · 2018-06-01T20:49:32Z

src/jit/lower.cpp

-#ifdef _TARGET_XARCH_
-    if (!varTypeIsFloating(node->TypeGet()))
-#endif // _TARGET_XARCH_
+    if (varTypeIsIntegral(node->TypeGet()))


I don't see a reason why it should be target-specific either.

sandreenko · 2018-06-01T23:19:02Z

Failures are not related with this change.

BruceForstall · 2018-06-29T23:11:01Z

@sandreenko Can you also re-enable it in issues.targets:

<!-- The following are ARM64_altjit crossgen failures. For x64_arm64_altjit buildArm==x64. -->

<ItemGroup Condition="'$(XunitTestBinBase)' != '' and '$(BuildArch)' == 'x64'">
    <ExcludeList Include="$(XunitTestBinBase)\JIT\Regression\JitBlue\DevDiv_590772\DevDiv_590772\DevDiv_590772.cmd">
        <Issue>17968</Issue>
    </ExcludeList>
...
</ItemGroup>

sandreenko · 2018-06-29T23:13:39Z

@sandreenko Can you also re-enable it in issues.targets:

Thanks, that was missed, I will publish the PR shortly.

Sergey Andreenko added 2 commits May 30, 2018 13:49

Fix the test

95e3528

It was copied wrong previously.

Disable a % b = a & (b - 1); optimization on arm64

bd05702

Morph can't relay on Lower optimization that it can't guarantee.

sandreenko added bug Product bug (most likely) area-CodeGen arch-arm64 labels May 30, 2018

sandreenko changed the title ~~Disable a % b = a & (b - 1); optimization on arm64~~ [arm64] Disable a % b = a & (b - 1); optimization. May 30, 2018

sandreenko closed this May 31, 2018

sandreenko reopened this May 31, 2018

improve checks in lower

61556ef

We do not expect MOD/UMOD for integer/long types in lower for arm64.

sandreenko force-pushed the Fix17968 branch from 920c26a to 61556ef Compare June 1, 2018 19:07

sandreenko commented Jun 1, 2018

View reviewed changes

CarolEidt approved these changes Jun 1, 2018

View reviewed changes

sandreenko merged commit ad92640 into dotnet:master Jun 1, 2018

sandreenko deleted the Fix17968 branch June 1, 2018 23:19

kunalspathak mentioned this pull request Apr 14, 2020

ARM64: Optimize a % b operation dotnet/runtime#34937

Closed

jakobbotsch mentioned this pull request Apr 24, 2022

Assertion failed '!"Shouldn't see an integer typed GT_MOD node in ARM64"' during 'Linear scan register alloc' dotnet/runtime#68470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[arm64] Disable `a % b = a & (b - 1);` optimization. #18206

[arm64] Disable `a % b = a & (b - 1);` optimization. #18206

sandreenko commented May 30, 2018 •

edited

Loading

sandreenko commented May 30, 2018 •

edited

Loading

mikedn commented May 30, 2018

sandreenko commented May 30, 2018

sandreenko commented May 30, 2018

mikedn commented May 30, 2018

sandreenko commented May 31, 2018

sandreenko commented Jun 1, 2018

sandreenko Jun 1, 2018

mikedn Jun 1, 2018

sandreenko Jun 1, 2018 •

edited

Loading

CarolEidt Jun 1, 2018

sandreenko commented Jun 1, 2018

CarolEidt Jun 1, 2018

sandreenko commented Jun 1, 2018

BruceForstall commented Jun 29, 2018

sandreenko commented Jun 29, 2018

[arm64] Disable a % b = a & (b - 1); optimization. #18206

[arm64] Disable a % b = a & (b - 1); optimization. #18206

Conversation

sandreenko commented May 30, 2018 • edited Loading

sandreenko commented May 30, 2018 • edited Loading

mikedn commented May 30, 2018

sandreenko commented May 30, 2018

sandreenko commented May 30, 2018

mikedn commented May 30, 2018

sandreenko commented May 31, 2018

sandreenko commented Jun 1, 2018

sandreenko Jun 1, 2018

Choose a reason for hiding this comment

mikedn Jun 1, 2018

Choose a reason for hiding this comment

sandreenko Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

CarolEidt Jun 1, 2018

Choose a reason for hiding this comment

sandreenko commented Jun 1, 2018

CarolEidt Jun 1, 2018

Choose a reason for hiding this comment

sandreenko commented Jun 1, 2018

BruceForstall commented Jun 29, 2018

sandreenko commented Jun 29, 2018

[arm64] Disable `a % b = a & (b - 1);` optimization. #18206

[arm64] Disable `a % b = a & (b - 1);` optimization. #18206

sandreenko commented May 30, 2018 •

edited

Loading

sandreenko commented May 30, 2018 •

edited

Loading

sandreenko Jun 1, 2018 •

edited

Loading