JIT: fix incorrect scale in genCreateAddrMode + no-opt by EgorBo · Pull Request #75433 · dotnet/runtime

EgorBo · 2022-09-12T04:56:55Z

The problem is basically an incorrect behavior of genCreateAddrMode for the following tree:

N009 ( 11, 9) [000008] -------N--- * ADD int
N007 ( 10, 8) [000006] -------N--- +--* ADD int
N001 ( 3, 2) [000000] -c--------- | +--* LCL_VAR int V00 arg0
N006 ( 7, 6) [000005] -------N--- | \--* LSH int
N004 ( 6, 5) [000003] ----------- | +--* MUL int
N002 ( 1, 1) [000001] ----------- | | +--* CNS_INT int 3
N003 ( 1, 1) [000002] -c--------- | | \--* CNS_INT int 0
N005 ( 1, 1) [000004] -c--------- | \--* CNS_INT int 2
N008 ( 1, 1) [000007] -c--------- \--* CNS_INT int 1

for which, genCreateAddrMode returns:

base = arg0
idx = nullptr
scale = 0
offset = 4

while offset should be 1

ghost · 2022-09-12T04:57:12Z

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

Issue Details

Fixes #75312

The problem is basically in a tree:

N009 ( 11, 9) [000008] -------N--- * ADD int
N007 ( 10, 8) [000006] -------N--- +--* ADD int
N001 ( 3, 2) [000000] -c--------- | +--* LCL_VAR int V00 arg0
N006 ( 7, 6) [000005] -------N--- | \--* LSH int
N004 ( 6, 5) [000003] ----------- | +--* MUL int
N002 ( 1, 1) [000001] ----------- | | +--* CNS_INT int 3
N003 ( 1, 1) [000002] -c--------- | | \--* CNS_INT int 0
N005 ( 1, 1) [000004] -c--------- | \--* CNS_INT int 2
N008 ( 1, 1) [000007] -c--------- \--* CNS_INT int 1

that is an address for IND, for which, genCreateAddrMode returns:

base = arg0
idx = nullptr
scale = 0
offset = 4

while offset should be 1

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

EgorBo · 2022-09-12T11:49:02Z

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

azure-pipelines · 2022-09-12T11:50:09Z

Azure Pipelines successfully started running 5 pipeline(s).

JulieLeeMSFT · 2022-09-12T15:27:51Z

@kunalspathak Please prioritize to review this PR before RC2 snap.
cc @dotnet/jit-contrib.

kunalspathak · 2022-09-12T15:33:28Z

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, runtime-coreclr jitstressregs, runtime-coreclr gcstress0x3-gcstress0xc, Fuzzlyn

Have you analyzed any of these failures and are there anything related?

JulieLeeMSFT · 2022-09-12T19:04:18Z

cc @jeffschwMSFT. Once we complete this PR, we will backport this to 7.0 and 6.0.

AndyAyersMS · 2022-09-12T19:19:49Z

Jit stress failures look like known issues:

Test failure JIT\\Performance\\CodeQuality\\Roslyn\\CscBench\\CscBench.cmd #74900 (fix in PR)
Test failure GC\\API\\GC\\GetTotalPauseDuration\\GetTotalPauseDuration.cmd #74631 (test was supposed to be disabled, but isn't yet)

AndyAyersMS · 2022-09-12T19:20:29Z

So the bug was an uninitialized local? How could we have not caught this before now?

kunalspathak · 2022-09-12T19:25:44Z

So the bug was an uninitialized local? How could we have not caught this before now?

That's exactly I was wondering. Why it needed complex C++ / CLI only to reproduce?

EgorBo · 2022-09-12T19:29:51Z

So the bug was an uninitialized local? How could we have not caught this before now?

Ah, no, that change is unrelated, let me remove the redundant initialization actually,
the problem is in zero scale, it's not respected.

EgorBo · 2022-09-12T19:32:00Z

Why it needed complex C++ / CLI only to reproduce?

It's not C++ specific, it's just that x * 0rarely makes it to genAddrMode phase - when optimizations are enabled it's folded long before it reaches the phase. In debug mode it's hard to get it anyhow since e.g. Roslyn folds x * 0 to 0 even in Debug mode with <Optimize>false</Optimize> - that's why I wrote the test in IL.

This bug is a good example on why we need unit tests for jit phases

…ebug

kunalspathak · 2022-09-12T19:57:17Z

the problem is in zero scale, it's not respected.

Make sense.

kunalspathak

LGTM

kunalspathak · 2022-09-12T20:42:43Z


 /* Scale the index if necessary */
- ixv *= tmpMul;
+ if (tmpMul)


So its ok to have tmpMul == 0 here from the else on line 1417 and not scale?

Looks like it can be but only if we also disable jit optimizations and write IL for array[1 + 2] because Roslyn will fold cns + cns too again (even in Debug mode).

Actually, let me run a few more tests, probably some parts of this logic can be just removed

@kunalspathak so I've slightly changed the logic - the whole block can be removed in theory but it produced several Tier0 regressions so I decided to keep it + special handled zero index or zero scale, so if optGetArrayRefScaleAndIndex returns either 0 or non-zero but index represents 0 then we drop the whole index part.

What happens for !(rv2->OperIs(GT_MUL, GT_LSH) && (rv2->gtGetOp2()->IsCnsIntOrI())) where we set index = rv2?
else { /* May be a simple array. rv2 will points to the actual index */ index = rv2; tmpMul = mul; }

@kunalspathak good point, it explains a single regression in the jit-diffs, but overall the code as is correct, it's just that the block you highlighted handles constant index that is converted to constant offset (this path is never hit in optimized code from what I see)

Let me push a quick change

@kunalspathak should be correct now and zero diff

kunalspathak

LGTM. Just double check if there are zero diffs for all archs before merging.

EgorBo · 2022-09-13T19:44:23Z

Yeah, the diff is empty but there is a conflict

EgorBo · 2022-09-13T20:34:44Z

/backport to release/7.0

github-actions · 2022-09-13T20:34:55Z

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3048213131

EgorBo · 2022-09-13T20:43:35Z

@kunalspathak could you also approve the backport - #75560

EgorBo added 2 commits September 12, 2022 06:44

Fix invalid addressing mode scale in debug mode
b3b02b3

Add a test
ef6929a

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

ghost assigned EgorBo Sep 12, 2022

EgorBo added 2 commits September 12, 2022 07:01

Update Runtime_75312.il
98136f4

Update Runtime_75312.ilproj
6a76f3e

EgorBo marked this pull request as ready for review September 12, 2022 12:55

JulieLeeMSFT added this to the 7.0.0 milestone Sep 12, 2022

JulieLeeMSFT requested a review from kunalspathak September 12, 2022 15:28

EgorBo added 2 commits September 12, 2022 21:36

Drop unneeded initialization
f4f9e96

Merge branch 'main' of github.com:dotnet/runtime into fix-addr-mode-d…
cbd353e
…ebug

kunalspathak approved these changes Sep 12, 2022
View reviewed changes

Slightly better fix
fb843cc

kunalspathak reviewed Sep 12, 2022
View reviewed changes

EgorBo added 3 commits September 12, 2022 23:51

Test an even better fix?
189c143

Refactoring
23f34d5

address feedback
0291b29

kunalspathak approved these changes Sep 13, 2022
View reviewed changes

Merge branch 'main' into fix-addr-mode-debug
c3f9e8a

github-actionsBot mentioned this pull request Sep 13, 2022
[release/7.0] JIT: fix incorrect scale in genCreateAddrMode + no-opt #75560
Merged

EgorBo merged commit 53815bd into dotnet:mainSep 13, 2022

ghost locked as resolved and limited conversation to collaborators Oct 14, 2022

Uh oh!

Conversation

EgorBo commented Sep 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 12, 2022

Uh oh!

EgorBo commented Sep 12, 2022

Uh oh!

azure-pipelinesBot commented Sep 12, 2022

Uh oh!

JulieLeeMSFT commented Sep 12, 2022

Uh oh!

kunalspathak commented Sep 12, 2022

Uh oh!

JulieLeeMSFT commented Sep 12, 2022

Uh oh!

AndyAyersMS commented Sep 12, 2022

Uh oh!

AndyAyersMS commented Sep 12, 2022

Uh oh!

kunalspathak commented Sep 12, 2022

Uh oh!

EgorBo commented Sep 12, 2022

Uh oh!

EgorBo commented Sep 12, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kunalspathak commented Sep 12, 2022

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

kunalspathakSep 12, 2022

Choose a reason for hiding this comment

Uh oh!

EgorBoSep 12, 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!

EgorBoSep 12, 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!

EgorBoSep 13, 2022

Choose a reason for hiding this comment

Uh oh!

kunalspathakSep 13, 2022

Choose a reason for hiding this comment

Uh oh!

EgorBoSep 13, 2022

Choose a reason for hiding this comment

Uh oh!

EgorBoSep 13, 2022

Choose a reason for hiding this comment

Uh oh!

EgorBoSep 13, 2022

Choose a reason for hiding this comment

Uh oh!

kunalspathak left a comment

Choose a reason for hiding this comment

Uh oh!

EgorBo commented Sep 13, 2022

Uh oh!

EgorBo commented Sep 13, 2022

Uh oh!

github-actionsBot commented Sep 13, 2022

Uh oh!

EgorBo commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

EgorBo commented Sep 12, 2022•
edited
Loading

EgorBo commented Sep 12, 2022•
edited
Loading

EgorBoSep 12, 2022•
edited
Loading

EgorBoSep 12, 2022•
edited
Loading