JIT: fix incorrect scale in genCreateAddrMode + no-opt #75433

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 * 0 rarely 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

src/coreclr/jit/codegencommon.cpp

            {
                /* Get hold of the index value */
                ssize_t ixv = index->AsIntConCommon()->IconValue();

                /* 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

dotnet-issue-labeler bot 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-actions bot 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:main Sep 13, 2022

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: fix incorrect scale in genCreateAddrMode + no-opt #75433

JIT: fix incorrect scale in genCreateAddrMode + no-opt #75433

EgorBo commented Sep 12, 2022 •

edited

Loading

ghost commented Sep 12, 2022

EgorBo commented Sep 12, 2022

azure-pipelines bot commented Sep 12, 2022

JulieLeeMSFT commented Sep 12, 2022

kunalspathak commented Sep 12, 2022

JulieLeeMSFT commented Sep 12, 2022

AndyAyersMS commented Sep 12, 2022

AndyAyersMS commented Sep 12, 2022

kunalspathak commented Sep 12, 2022

EgorBo commented Sep 12, 2022

EgorBo commented Sep 12, 2022 •

edited

Loading

kunalspathak commented Sep 12, 2022

kunalspathak left a comment

kunalspathak Sep 12, 2022

EgorBo Sep 12, 2022 •

edited

Loading

EgorBo Sep 12, 2022 •

edited

Loading

EgorBo Sep 13, 2022

kunalspathak Sep 13, 2022

EgorBo Sep 13, 2022

EgorBo Sep 13, 2022

EgorBo Sep 13, 2022

kunalspathak left a comment

EgorBo commented Sep 13, 2022

EgorBo commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

EgorBo commented Sep 13, 2022

JIT: fix incorrect scale in genCreateAddrMode + no-opt #75433

JIT: fix incorrect scale in genCreateAddrMode + no-opt #75433

Conversation

EgorBo commented Sep 12, 2022 • edited Loading

ghost commented Sep 12, 2022

EgorBo commented Sep 12, 2022

azure-pipelines bot commented Sep 12, 2022

JulieLeeMSFT commented Sep 12, 2022

kunalspathak commented Sep 12, 2022

JulieLeeMSFT commented Sep 12, 2022

AndyAyersMS commented Sep 12, 2022

AndyAyersMS commented Sep 12, 2022

kunalspathak commented Sep 12, 2022

EgorBo commented Sep 12, 2022

EgorBo commented Sep 12, 2022 • edited Loading

kunalspathak commented Sep 12, 2022

kunalspathak left a comment

Choose a reason for hiding this comment

kunalspathak Sep 12, 2022

Choose a reason for hiding this comment

EgorBo Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

EgorBo Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

EgorBo Sep 13, 2022

Choose a reason for hiding this comment

kunalspathak Sep 13, 2022

Choose a reason for hiding this comment

EgorBo Sep 13, 2022

Choose a reason for hiding this comment

EgorBo Sep 13, 2022

Choose a reason for hiding this comment

EgorBo Sep 13, 2022

Choose a reason for hiding this comment

kunalspathak left a comment

Choose a reason for hiding this comment

EgorBo commented Sep 13, 2022

EgorBo commented Sep 13, 2022

github-actions bot commented Sep 13, 2022

EgorBo commented Sep 13, 2022

EgorBo commented Sep 12, 2022 •

edited

Loading

EgorBo commented Sep 12, 2022 •

edited

Loading

EgorBo Sep 12, 2022 •

edited

Loading

EgorBo Sep 12, 2022 •

edited

Loading