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.

Sign up for GitHub

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

Jump to bottom

JIT: Fold "shift-by-zero" in lower #61222

Merged
merged 4 commits into from
Nov 5, 2021
Merged

JIT: Fold "shift-by-zero" in lower #61222

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f653bf2
fold "shift-by-zero" in lower
EgorBo Nov 4, 2021
2e8f67a
oh, the test actually throws NRE
EgorBo Nov 4, 2021
2240c3a
Update Runtime_61045.cs
EgorBo Nov 4, 2021
d6f463d
Update lower.cpp
EgorBo Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5754,6 +5754,16 @@ void Lowering::LowerShift(GenTreeOp* shift)
}
ContainCheckShiftRotate(shift);

// Fold any kind of shift by zero, e.g. LSH(X, 0) => X
LIR::Use use;
if (shift->gtGetOp2()->IsIntegralConst(0) && BlockRange().TryGetUse(shift, &use))
{
use.ReplaceWith(shift->gtGetOp1());
BlockRange().Remove(shift->gtGetOp2());
BlockRange().Remove(shift);
return;
}

#ifdef TARGET_ARM64
// Try to recognize ubfiz/sbfiz idiom in LSH(CAST(X), CNS) tree
if (comp->opts.OptimizationEnabled() && shift->OperIs(GT_LSH) && shift->gtGetOp1()->OperIs(GT_CAST) &&
Expand All @@ -5773,7 +5783,7 @@ void Lowering::LowerShift(GenTreeOp* shift)
assert(!cast->CastOp()->isContained());

// It has to be an upcast and CNS must be in [1..srcBits) range
if ((srcBits < dstBits) && ((UINT32)cns->IconValue() < srcBits))
if ((srcBits < dstBits) && (cns->IconValue() > 0) && (cns->IconValue() < srcBits))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I understand correctly this is the fix and the removal of nop shifts is a separate optimization, or is that required as well?

FWIW, I would prefer we not do these ad-hoc opts in lowering (instead tracing back the where the frontend "failed at its job" and so on).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SingleAccretion it's pretty normal to duplicate peepholes accross phases and we already do it. Front-end didn't fail here, only rationalizer managed to fold this:

N036 ( 47, 59) [000024] -ACXG-------              \--*  LSH       int    <l:$2c8, c:$2d2>
N035 (  2,  5) [000067] -A--G-------                 \--*  COMMA     int   
N033 (  1,  3) [000042] -A--G---R---                    +--*  ASG       ref    <l:$1c2, c:$1d5>
N032 (  1,  1) [000041] D------N----                    |  +--*  LCL_VAR   ref    V02 tmp2         d:1 <l:$280, c:$85>
N031 (  1,  1) [000060] ------------                    |  \--*  LCL_VAR   ref    V03 cse0         u:1 <l:$280, c:$81>
N034 (  1,  2) [000066] ------------                    \--*  CNS_INT   int    0 <l:$2d1, c:$2d0>

into

N033 (  1,  3) [000042] DA--G-------              *  STORE_LCL_VAR ref    V02 tmp2         d:1
N034 (  1,  2) [000066] ------------        t66 =    CNS_INT   int    0 <l:$2d1, c:$2d0>
                                                  /--*  t9     int    
                                                  +--*  t66    int    
N036 ( 47, 59) [000024] ---XG-------        t24 = *  LSH       int    <l:$2c8, c:$2d2>
                                                  /--*  t24    int    
N037 ( 48, 60) [000025] ---XG-------              *  RETURN    int    $107

Copy link
Member Author

@EgorBo EgorBo Nov 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe some day we'll get a DSL for transformations which will be compiled into morph and lower at the same time (that's how some C++ compilers work today)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really worth to add this optimization in lower that only has hits in fuzzer generated code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakobbotsch not really, but the diff found 4 more cases and we still don't have collections for real-world apps to judge. but let me remove it since it raised concerns by you two

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PTAL

{
JITDUMP("Recognized ubfix/sbfix pattern in LSH(CAST, CNS). Changing op to GT_BFIZ");
shift->ChangeOper(GT_BFIZ);
Expand Down
37 changes: 37 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_61045/Runtime_61045.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Generated by Fuzzlyn v1.5 on 2021-11-04 18:29:11
// Run on Arm64 Linux
// Seed: 1922924939431163374
// Reduced from 261.1 KiB to 0.2 KiB in 00:05:39
// Hits JIT assert in Release:
// Assertion failed 'isValidImmShift(lsb, size)' in 'Program:M4():int' during 'Generate code' (IL size 26)
//
// File: /__w/1/s/src/coreclr/jit/emitarm64.cpp Line: 7052
//

using System.Runtime.CompilerServices;

public class Runtime_61045
{
public static byte[] s_1;
public static int Main()
{
try
{
Test();
}
catch (NullReferenceException)
{
return 100;
}
return 101;
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static uint Test()
{
return (uint)((ushort)~s_1[0] << (0 >> s_1[0]));
}
}
9 changes: 9 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_61045/Runtime_61045.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>