Fix VN incorrect optimizations with a new JitEEInterface function. #57282

sandreenko · 2021-08-12T12:57:55Z

This is an alternative for #57076 that gets VM help to avoid non-unique fieldSeq in VN maps.

The main advantage of this approach is that we keep ValueNumbering changes small (and I personally can't be confident in any VN changes) and we don't change other phases to carry any information that was not there. Instead, we ask JitEEInterface where we need to restore field->struct information.

Thanks to @davidwrighton for the VM part of this change.

No pmi diffs, can't do SPMI because the GUID has changed. Will post crossgen2 diffs later, we expect small regressions there.

Fixes #42517, fixes #49954, fixes #54102, fixes #56980.

#57076 (comment) has a detailed explanation of the issue, but it is probably faster to look at the test cases added in this PR.

sandreenko · 2021-08-16T09:43:15Z

/azp run runtime-coreclr libraries-jitstress, runtime-coreclr jitstress, runtime-coreclr outerloop

azure-pipelines · 2021-08-16T09:43:55Z

Azure Pipelines successfully started running 3 pipeline(s).

sandreenko · 2021-08-16T09:52:39Z

src/coreclr/jit/valuenum.cpp

@@ -8006,10 +8006,11 @@ void Compiler::fgValueNumberTree(GenTree* tree)
                                        rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet()));
                                    }

+                                    lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);


For a tree like:

N005 ( 11, 10) [001400] -A------R---- * ASG long N004 ( 6, 5) [001401] *------N----- +--* IND long N003 ( 3, 3) [001402] ------------- | \--* ADDR byref Zero Fseq[_00] N002 ( 3, 2) [001403] U------N----- | \--* LCL_VAR struct<System.Runtime.Intrinsics.Vector128`1[Byte], 16> V45 tmp38 ud:3->4 N001 ( 1, 1) [001404] ------------- \--* LCL_VAR long V69 tmp62 u:2 (last use)

SSA was working correctly and printing that 001403 is using SSA-3 and defining SSA-4. However, this code, for some reason, was writing the result as a define for SSA-3.

So when at the next tree we were asking for GetPerSsaData(4) we were not getting an initialized value.

Maybe this comes up now because we no longer set lvOverlappingFields in the importer?

It looks like this could be moved to come before line 8051, as that is the first use of this value,

sandreenko · 2021-08-16T09:56:11Z

src/coreclr/jit/valuenum.cpp

@@ -7273,6 +7269,29 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
                    isNewUniq = true;
                }

+                if (!isNewUniq && (rhsVarDsc != nullptr) && (lhsVarDsc != nullptr))


the check isNewUniq is weak here because it is possible that we have already generated a unique VN for rhs but did not report it here.
Because of that we can't narrow the cases that go inside this block to assert(ClassLayout::AreCompatible(rhsVarDsc->GetLayout(), lhsVarDsc->GetLayout())).
However, all cases that pass all these "if" should have unique VN, we just ask for it twice for some trees.

sandreenko · 2021-08-16T09:56:38Z

PTAL @AndyAyersMS @briansull @dotnet/jit-contrib

src/tests/JIT/Regression/JitBlue/Runtime_57282/Runtime_57282_2.cs

AndyAyersMS

Overall this looks to be in good shape.

Left you a few notes.

AndyAyersMS · 2021-08-16T18:22:48Z

src/coreclr/jit/valuenum.cpp

@@ -8006,10 +8006,11 @@ void Compiler::fgValueNumberTree(GenTree* tree)
                                        rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet()));
                                    }

+                                    lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);


Maybe this comes up now because we no longer set lvOverlappingFields in the importer?

AndyAyersMS · 2021-08-16T18:33:33Z

src/coreclr/jit/valuenum.cpp

+                            }
+                            else
+                            {
+                                CORINFO_FIELD_HANDLE fldHnd = rhsFldSeq->GetTail()->GetFieldHandle();


Presumably we've weeded out the NotAField cases above?

Maybe assert for it here, as calling getFieldClass on a null handle will cause an AV.

AndyAyersMS · 2021-08-16T18:33:47Z

src/coreclr/jit/valuenum.cpp

+                            }
+                            else
+                            {
+                                CORINFO_FIELD_HANDLE fldHnd = lhsFldSeq->GetTail()->GetFieldHandle();


AndyAyersMS · 2021-08-16T18:41:30Z

src/coreclr/jit/valuenum.cpp

+                            else
+                            {
+                                CORINFO_FIELD_HANDLE fldHnd = rhsFldSeq->GetTail()->GetFieldHandle();
+                                rhsStructHnd                = info.compCompHnd->getFieldClass(fldHnd);


There's a bit of a divergence between doesFieldBelongToClass and getFieldClass -- otherwise you would not have needed this new interface method. Can that cause problems here?

AndyAyersMS · 2021-08-16T18:48:31Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs

+            // and logical field pair then return true. This is needed as the field handle here
+            // is used as a key into a hashtable mapping writes to fields to value numbers.
+            //
+            // In this implmentation this is made more complex as the JIT is exposed to CORINFO_FIELD_STRUCT


Maybe make it clearer that the crossgen2 behavior is different than the runtime behavior?

Is this something we should fix down the road? Seems like it would make us more conservative in places when doing R2R codegen.

Maybe make it clearer that the crossgen2 behavior is different than the runtime behavior?

I will add a comment.

Is this something we should fix down the road? Seems like it would make us more conservative in places when doing R2R codegen.

I hope we will target to get rid of FldSeq and its dependency on FIELD_HANDLE and STRUCT_HANDLE.

If not we can do:

if (crossgen2) { canonFieldHandle = JitEEInterface::getCanonField(fieldHandle); bool doesBelong = JitEEInterface::doesFieldBelongToClass(canonFieldHandle , clsHnd); }

but I don't like such a solution, it hammers JIT to VM behavior, we can do it always, not only for crossgen2, but then it is an unnecessary VM call.

AndyAyersMS · 2021-08-16T18:51:03Z

src/coreclr/jit/lclmorph.cpp

+
+                                haveCorrectFieldForVN =
+                                    compiler->info.compCompHnd->doesFieldBelongToClass(field->gtFldHnd, clsHnd);
+                                assert(haveCorrectFieldForVN);


If we're going to assert like this (as opposed to say noway which might be appropriate here, if we think the only ways we can get confused about fields is when optimizing) then you might as well only call the interface method under DEBUG.

I thought that when using UnsafeAs to change the struct type to a different struct type, we would get a false back from doesFieldBelongToClass

changed to a noway assert.

I thought that when using UnsafeAs to change the struct type to a different struct type, we would get a false back from doesFieldBelongToClass

yes, but they have to stay in the same tree without spilling, so something like:

struct1 s1 = new S1(); Unsafe.As<s1, s2>(ref s1).fieldOfS2 = 10;

if this is imported as one field:

FIELD fieldOfS2 \-- OBJ(struct2) \-- ADDR byref \--LCL_VAR struct1

here we should catch it and transform as

LCL_FLD struct1 (NotAField)

instead of

LCL_FLD struct1 (fieldOfS2)

but I could not create a test where we keep the source as 1 tree so I added an assert that it is unreachable for now.

briansull · 2021-08-16T22:50:45Z

src/coreclr/jit/lclmorph.cpp

+
+                                haveCorrectFieldForVN =
+                                    compiler->info.compCompHnd->doesFieldBelongToClass(field->gtFldHnd, clsHnd);
+                                assert(haveCorrectFieldForVN);


I thought that when using UnsafeAs to change the struct type to a different struct type, we would get a false back from doesFieldBelongToClass

briansull · 2021-08-16T23:05:23Z

src/coreclr/jit/valuenum.cpp

@@ -8006,10 +8006,11 @@ void Compiler::fgValueNumberTree(GenTree* tree)
                                        rhsVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet()));
                                    }

+                                    lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);


It looks like this could be moved to come before line 8051, as that is the first use of this value,

src/coreclr/tools/Common/JitInterface/CorInfoBase.cs

sandreenko · 2021-08-23T16:37:20Z

/azp run runtime-coreclr libraries-jitstress, runtime-coreclr jitstress, runtime-coreclr outerloop

azure-pipelines · 2021-08-23T16:38:01Z

Azure Pipelines successfully started running 3 pipeline(s).

sandreenko · 2021-08-24T00:15:06Z

/azp run runtime-coreclr libraries-jitstress, runtime-coreclr jitstress, runtime-coreclr outerloop

azure-pipelines · 2021-08-24T00:15:44Z

Azure Pipelines successfully started running 3 pipeline(s).

with a naive implementation so far.

… type case in the managed compiler

It appeared that we have many trees where we generate unique VN for rhs and don't inform `fgValueNumberBlockAssignment` about it. For example, for ``` N005 ( 10, 8) [000033] -A--G---R--- * ASG struct (copy) N004 ( 3, 2) [000031] D------N---- +--* LCL_VAR struct<eightByteStruct, 8> V01 loc1 d:2 N003 ( 6, 5) [000030] n---G------- \--* IND struct N002 ( 3, 3) [000043] ------------ \--* ADDR byref N001 ( 3, 2) [000044] -------N---- \--* LCL_VAR struct<largeStruct, 32> V00 loc0 u:6 (last use) ``` we will generate unique rhs but then `fgValueNumberBlockAssignment` will still try to work it as with a non-unique one.

For a tree like: ``` N005 ( 11, 10) [001400] -A------R---- * ASG long N004 ( 6, 5) [001401] *------N----- +--* IND long N003 ( 3, 3) [001402] ------------- | \--* ADDR byref Zero Fseq[_00] N002 ( 3, 2) [001403] U------N----- | \--* LCL_VAR struct<System.Runtime.Intrinsics.Vector128`1[Byte], 16> V45 tmp38 ud:3->4 N001 ( 1, 1) [001404] ------------- \--* LCL_VAR long V69 tmp62 u:2 (last use) ``` SSA was working correctly and printing that `001403` is using SSA-3 and defining SSA-4. However, this code, for some reason, was writing result as a define for SSA-3.

sandreenko · 2021-08-24T06:06:03Z

/azp run runtime-coreclr libraries-jitstress, runtime-coreclr jitstress, runtime-coreclr outerloop

azure-pipelines · 2021-08-24T06:06:37Z

Azure Pipelines successfully started running 3 pipeline(s).

sandreenko · 2021-08-24T09:59:12Z

The only failure is test45929 but it was failing like this in other test runs (checked with kusto), for example, in https://dev.azure.com/dnceng/public/_build/results?buildId=1277844&view=ms.vss-test-web.build-test-results-tab&runId=37787400&resultId=110575&paneView=debug.

sandreenko · 2021-08-24T10:00:02Z

/backport to release/6.0-rc1

github-actions · 2021-08-24T10:00:17Z

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1162100281

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 12, 2021

sandreenko force-pushed the tryToAddNewMethod branch 3 times, most recently from 0f34021 to 397825b Compare August 15, 2021 22:23

sandreenko changed the title ~~test.~~ Fix VN incorrect optimizations with a new JitEEInterface function. Aug 15, 2021

sandreenko mentioned this pull request Aug 15, 2021

Avoid VN bugs with struct reinterpretation. #57076

Closed

sandreenko marked this pull request as ready for review August 16, 2021 09:43

dotnet deleted a comment from azure-pipelines bot Aug 16, 2021

sandreenko commented Aug 16, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

This was referenced Aug 16, 2021

System.ComponentModel.Win32Exception thrown in System.Net.Security.Tests #52361

Closed

System.Net.Security.Tests failed with System.IO.IOException #52362

Closed

JulieLeeMSFT requested review from briansull and AndyAyersMS August 16, 2021 16:41

JulieLeeMSFT assigned sandreenko Aug 16, 2021

JulieLeeMSFT added this to the 6.0.0 milestone Aug 16, 2021

jakobbotsch reviewed Aug 16, 2021

View reviewed changes

src/tests/JIT/Regression/JitBlue/Runtime_57282/Runtime_57282_2.cs Show resolved Hide resolved

AndyAyersMS reviewed Aug 16, 2021

View reviewed changes

briansull mentioned this pull request Aug 16, 2021

Clean-up VN for promoted fields assigned using the whole parent. #41242

Closed

4 tasks

briansull approved these changes Aug 16, 2021

View reviewed changes

jaredpar mentioned this pull request Aug 17, 2021

Feed unreliability affecting CI #55449

Closed

runfoapp bot mentioned this pull request Aug 17, 2021

Test failure in System.Diagnostics.Process.Tests #57452

Closed

sandreenko force-pushed the tryToAddNewMethod branch from 1a8c70a to cf38484 Compare August 23, 2021 06:01

runfoapp bot mentioned this pull request Aug 23, 2021

Can_Run_App_With_StatiHost installer test failure in CI #42101

Closed

Sergey and others added 10 commits August 23, 2021 20:45

Add/return tests.

d579fe5

improve the test naming.

56361dd

Add a new JitEE method.

f239a37

with a naive implementation so far.

Handle generically dissimilar type concerns as well as handle derived…

f40a7dc

… type case in the managed compiler

Fix the field and Unsafe issues.

fb0e901

Add new test cases and a fix for them.

a4add9d

correct a test copy paste

fe65446

response review.

e438e3d

sandreenko force-pushed the tryToAddNewMethod branch from 0cffe6a to e438e3d Compare August 24, 2021 03:46

sandreenko merged commit bc7081e into dotnet:main Aug 24, 2021

github-actions bot mentioned this pull request Aug 24, 2021

[release/6.0] Fix VN incorrect optimizations with a new JitEEInterface function. #58005

Merged

runfoapp bot mentioned this pull request Aug 24, 2021

Recurring test failures in tracing due to class loader alloc tracker heap corruption #57461

Closed

karelz mentioned this pull request Aug 30, 2021

ClientAndServer_OneOrBothUseDefault_Ok failed in CI with ObjectDisposedException #30724

Closed

ghost locked as resolved and limited conversation to collaborators Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix VN incorrect optimizations with a new JitEEInterface function. #57282

Fix VN incorrect optimizations with a new JitEEInterface function. #57282

sandreenko commented Aug 12, 2021 •

edited

Loading

sandreenko commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

sandreenko Aug 16, 2021

AndyAyersMS Aug 16, 2021

briansull Aug 16, 2021

sandreenko Aug 16, 2021

sandreenko commented Aug 16, 2021

This comment has been minimized.

This comment has been minimized.

AndyAyersMS left a comment

AndyAyersMS Aug 16, 2021

AndyAyersMS Aug 16, 2021

AndyAyersMS Aug 16, 2021

AndyAyersMS Aug 16, 2021

AndyAyersMS Aug 16, 2021

sandreenko Aug 22, 2021

AndyAyersMS Aug 16, 2021

briansull Aug 16, 2021

sandreenko Aug 22, 2021

briansull Aug 16, 2021

briansull Aug 16, 2021

sandreenko commented Aug 23, 2021

azure-pipelines bot commented Aug 23, 2021

sandreenko commented Aug 24, 2021

azure-pipelines bot commented Aug 24, 2021

sandreenko commented Aug 24, 2021

azure-pipelines bot commented Aug 24, 2021

sandreenko commented Aug 24, 2021

sandreenko commented Aug 24, 2021

github-actions bot commented Aug 24, 2021

Fix VN incorrect optimizations with a new JitEEInterface function. #57282

Fix VN incorrect optimizations with a new JitEEInterface function. #57282

Conversation

sandreenko commented Aug 12, 2021 • edited Loading

sandreenko commented Aug 16, 2021

azure-pipelines bot commented Aug 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Aug 16, 2021

This comment has been minimized.

This comment has been minimized.

AndyAyersMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandreenko commented Aug 23, 2021

azure-pipelines bot commented Aug 23, 2021

sandreenko commented Aug 24, 2021

azure-pipelines bot commented Aug 24, 2021

sandreenko commented Aug 24, 2021

azure-pipelines bot commented Aug 24, 2021

sandreenko commented Aug 24, 2021

sandreenko commented Aug 24, 2021

github-actions bot commented Aug 24, 2021

sandreenko commented Aug 12, 2021 •

edited

Loading