[JIT] - Fixed sub-optimal optimization for `a % 1` to Zero by TIHan · Pull Request #77760 · dotnet/runtime

TIHan · 2022-11-02T00:58:00Z

Description

Should resolve: #10956

We already did the transformation of a % 1, but we ignored it if op1 had any side effects. This PR should fix that.

X64 Diff:

Acceptance Criteria

Disasm tests

ghost · 2022-11-02T00:58:14Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

** Description **

Should resolve: #10956

Simple optimization. Will transform a % 1 to be 0.

** Acceptance Criteria **

Disasm tests

Author:	TIHan
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

src/coreclr/jit/morph.cpp

SingleAccretion · 2022-11-02T06:29:43Z

Note there is already an optimization that does this:

runtime/src/coreclr/jit/morph.cpp

Lines 9872 to 9887 in bdd67af

    
                       // For "val % 1", return 0 if op1 doesn't have any side effects 
        
                       // and we are not in the CSE phase, we cannot discard 'tree' 
        
                       // because it may contain CSE expressions that we haven't yet examined. 
        
                       // 
        
                       if (((op1->gtFlags & GTF_SIDE_EFFECT) == 0) && !optValnumCSE_phase) 
        
                       { 
        
                           if (op2->IsIntegralConst(1)) 
        
                           { 
        
                               GenTree* zeroNode = gtNewZeroConNode(typ); 
        
           #ifdef DEBUG 
        
                               zeroNode->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; 
        
           #endif 
        
                               DEBUG_DESTROY_NODE(tree); 
        
                               return zeroNode; 
        
                           } 
        
                       }

.

TIHan · 2022-11-02T06:46:10Z

Good catch. Will probably remove it in favor of this since it expands it.

TIHan · 2022-11-03T03:41:27Z

@dotnet/jit-contrib This is ready.

jakobbotsch · 2022-11-03T22:04:32Z

/azp run Fuzzlyn

azure-pipelines · 2022-11-03T22:04:47Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2022-11-04T09:55:11Z

The failures look related

TIHan · 2022-11-05T02:36:39Z

@dotnet/jit-contrib @jakobbotsch This is ready again.

src/coreclr/jit/morph.cpp

TIHan · 2022-11-14T18:43:51Z

@dotnet/jit-contrib @EgorBo This is ready for review again. We update the value number for the zero node, and we disable the optimization if we are not in global morph when there are side effects.

src/coreclr/jit/morph.cpp

TIHan · 2022-11-15T19:05:55Z

@dotnet/jit-contrib ready again

TIHan · 2022-11-28T21:00:11Z

@dotnet/jit-contrib @jakobbotsch @EgorBo This is ready again too. Fixed a minor issue when setting the integral value for GT_CNS_LNG on x86/arm.

src/tests/JIT/opt/Remainder/Regressions/Regression1/Regression1.csproj

…l not happen if optimizations are disabled

TIHan · 2022-12-02T18:34:48Z

@dotnet/jit-contrib This is ready for another round of review, based on our internal discussion yesterday.

BruceForstall · 2022-12-05T22:56:30Z

src/tests/JIT/opt/Remainder/IntRemainder.csproj

+    <CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
+    <CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />


One thing I don't remember:

If we have <HasDisasmCheck>true<...>, why do we need to clear TieredCompilation/JITMinOpts? Shouldn't the "HasDisasmCheck" logic do that for us?

<HasDisasmCheck>true<...> does not force any environment variables that affect codegen - so it is up to the test itself to decide them.

Optimize a % 1 to Zero

20f9bd0

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 2, 2022

ghost assigned TIHan Nov 2, 2022

Update disasm tests

b16ce6f

TIHan commented Nov 2, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

TIHan changed the title ~~Optimize a % 1 to Zero~~ [JIT] - Optimize a % 1 to Zero Nov 2, 2022

Minor change

71c19d1

Remove duplicate opt

ad1fab7

TIHan changed the title ~~[JIT] - Optimize a % 1 to Zero~~ [JIT] - Fixed sub-optimal optimization for a % 1 to Zero Nov 2, 2022

Use extract side effects

daaa0c4

TIHan marked this pull request as ready for review November 2, 2022 22:24

TIHan added 2 commits November 2, 2022 16:54

Re-arranging mod optimizations

79aff96

Re-arranging mod optimizations

3428879

build-analysis bot mentioned this pull request Nov 3, 2022

Tracking issue for CI build timeouts #76454

Closed

runfoapp bot mentioned this pull request Nov 3, 2022

Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040

Closed

build-analysis bot mentioned this pull request Nov 4, 2022

Tracking Nuget 429s dotnet/arcade#10885

Closed

2 tasks

TIHan added 2 commits November 4, 2022 18:55

Merge remote-tracking branch 'upstream/main' into mod-1-opt

46413df

Extended mod optimization with -1. Unsetting debug node morphed flag.

a6af61e

TIHan changed the title ~~[JIT] - Fixed sub-optimal optimization for a % 1 to Zero~~ [JIT] - Fixed sub-optimal optimization for a % 1 and a % -1 to Zero Nov 5, 2022

Remove space

ea19049

TIHan added 2 commits November 7, 2022 11:10

Try catch the regression

9c1d679

Use FULL-LINE for tests

8b11f51

jakobbotsch reviewed Nov 7, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

jakobbotsch reviewed Nov 7, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Show resolved Hide resolved

SingleAccretion reviewed Nov 7, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

Remove '% -1' opt

c757925

TIHan changed the title ~~[JIT] - Fixed sub-optimal optimization for a % 1 and a % -1 to Zero~~ [JIT] - Fixed sub-optimal optimization for a % 1 to Zero Nov 7, 2022

TIHan added 3 commits November 8, 2022 11:32

Updated vn for constant. Re-using op2 for zero constant.

87fdc1d

Merge remote-tracking branch 'upstream/main' into mod-1-opt

60835fd

Do not optimize part of this if not global morph

05086f9

SingleAccretion reviewed Nov 14, 2022

View reviewed changes

src/coreclr/jit/morph.cpp Outdated Show resolved Hide resolved

Checking side effect flag early

84f12f4

Check for GT_CNS_LNG

41f3a56

Update Regression1.csproj

4b8bccc

markples reviewed Nov 29, 2022

View reviewed changes

src/tests/JIT/opt/Remainder/Regressions/Regression1/Regression1.csproj Outdated Show resolved Hide resolved

TIHan added 6 commits November 29, 2022 18:54

Recording issue for mono

eb6f860

Update issues.targets

6504bcd

Using SetIntegralValue, removed unrelated changes, transformation wil…

7bc349d

…l not happen if optimizations are disabled

Minor change

0c225ee

Minor change

475e4d6

Use IsIntegralConst

2c37388

BruceForstall approved these changes Dec 6, 2022

View reviewed changes

TIHan merged commit 932221d into dotnet:main Dec 8, 2022

jakobbotsch mentioned this pull request Dec 12, 2022

Assertion failed '((tree->gtDebugFlags & GTF_DEBUG_NODE_MORPHED) == 0) && "ERROR: Already morphed this node!"' during 'Morph - Global' #79543

Closed

ghost locked as resolved and limited conversation to collaborators Jan 9, 2023

		<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
		<CLRTestEnvironmentVariable Include="DOTNET_JITMinOpts" Value="0" />

Conversation

TIHan commented Nov 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Nov 2, 2022

Uh oh!

Uh oh!

SingleAccretion commented Nov 2, 2022

Uh oh!

TIHan commented Nov 2, 2022

Uh oh!

TIHan commented Nov 3, 2022

Uh oh!

jakobbotsch commented Nov 3, 2022

Uh oh!

azure-pipelines bot commented Nov 3, 2022

Uh oh!

jakobbotsch commented Nov 4, 2022

Uh oh!

TIHan commented Nov 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TIHan commented Nov 14, 2022

Uh oh!

Uh oh!

TIHan commented Nov 15, 2022

Uh oh!

TIHan commented Nov 28, 2022

Uh oh!

Uh oh!

TIHan commented Dec 2, 2022

Uh oh!

BruceForstall Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

TIHan Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TIHan commented Nov 2, 2022 •

edited

Loading

TIHan Dec 8, 2022 •

edited

Loading