ValueNum add check for ZeroOffsetFldSeq on LclVar reads #20838

briansull · 2018-11-06T18:05:22Z

Call ExtendPtrVN with any ZeroOffsetFldSeq on LclVar reads
Fixes issue #18259

briansull · 2018-11-07T21:20:47Z

Desktop Asm Diffs are minimal:
We now occasionally can perform an additional CSE

     BING_1_clean_thin_unique_all.dasm                         3296        3298          -2         -0.06         2
        2 functions regressed (2 total bytes regression)
     CLR_SH_2_clean_thin_unique_all.dasm                        185         188          -3         -1.62         1
        1 functions regressed (3 total bytes regression)
           Regression # 1 goes from   185 to   188, diff of     3 ( 1.60%) :: System.Collections.Generic.GenericArraySortHelper`1[TimeSpan][System.TimeSpan]:BinarySearch(ref,int,int,struct):int
     CLR_SH_4_clean_thin_unique_all.dasm                        185         188          -3         -1.62         1
        1 functions regressed (3 total bytes regression)
           Regression # 1 goes from   185 to   188, diff of     3 ( 1.60%) :: System.Collections.Generic.GenericArraySortHelper`1[TimeSpan][System.TimeSpan]:BinarySearch(ref,int,int,struct):int
     CLR_SH_5_clean_thin_unique_all.dasm                       5880        5880           0          0.00        12
     CLR_SH_6_clean_thin_unique_all.dasm                        117         120          -3         -2.56         1
        1 functions regressed (3 total bytes regression)
           Regression # 1 goes from   117 to   120, diff of     3 ( 2.50%) :: System.Collections.Generic.GenericArraySortHelper`1[TimeSpan][System.TimeSpan]:BinarySearch(ref,int,int,struct):int
     CLR_SH_7_clean_thin_unique_all.dasm                        117         120          -3         -2.56         1
        1 functions regressed (3 total bytes regression)
           Regression # 1 goes from   117 to   120, diff of     3 ( 2.50%) :: System.Collections.Generic.GenericArraySortHelper`1[TimeSpan][System.TimeSpan]:BinarySearch(ref,int,int,struct):int
     CLR_ST_clean_thin_unique_all.dasm                         1530        1530           0          0.00         2
     CQ_PERF_clean_thin_unique_all.dasm                        1830        1795          35          1.91         1
        1 functions improved (35 total bytes improvement)
           Improvement # 1 goes from  1830 to  1795, diff of    35 ( 1.91%) :: Crafty.Net.OO.Board:SetBoard(int,ref,bool):this
     all_fx_clean_thin_unique_all.dasm                         1924        1925          -1         -0.05         3
        1 functions improved (3 total bytes improvement)
           Improvement # 1 goes from  1032 to  1029, diff of     3 ( 0.29%) :: System.ServiceModel.Channels.SequenceRangeCollection:GeneralMerge(ref,struct):ref
        2 functions regressed (4 total bytes regression)
           Regression # 1 goes from   408 to   410, diff of     2 ( 0.49%) :: System.ServiceModel.Channels.MessageProperties:CopyProperties(ref):this
           Regression # 2 goes from   484 to   486, diff of     2 ( 0.41%) :: System.ServiceModel.Channels.MessageProperties:MergeProperties(ref):this
     mscorlib_clean_thin_unique_all.dasm                        117         120          -3         -2.56         1
        1 functions regressed (3 total bytes regression)
           Regression # 1 goes from   117 to   120, diff of     3 ( 2.50%) :: System.Collections.Generic.GenericArraySortHelper`1[TimeSpan][System.TimeSpan]:BinarySearch(ref,int,int,struct):int
     TOTAL                                                    15181       15164          17          0.11        25

briansull · 2018-11-07T21:21:27Z

@AndyAyersMS PTAL
/cc: @dotnet/jit-contrib

briansull · 2018-11-07T21:21:45Z

I will add a test case

briansull · 2018-11-07T22:26:23Z

Test case added

CarolEidt

I just have some questions ...

CarolEidt · 2018-11-07T23:03:48Z

src/jit/valuenum.cpp

-                        assert((nextNode->gtOper == GT_ADDR && nextNode->gtOp.gtOp1 == lcl) ||
-                               varTypeIsStruct(lcl->TypeGet()));
-                        lcl->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lcl->TypeGet()));
+                        // Temporary, to make progress.


I realize that this was pre-existing, but this comment should be more detailed, and describe why this is temporary, and what kind of an assert should be re-added (i.e. should we be asserting that it's not NoVN at this point?)
Also, AFAICT this code section is the only substantive change. When making lots of refactoring/renaming changes, it would be really useful to call attention to where the real changes are and/or to have them in separate commits.

OK. I will do that in the future. You are correct that the fix is due to the change to

Call ExtendPtrVN with any ZeroOffsetFldSeq on LclVar reads

At thsi point I believe that this comment should just be removed:
// TODO-CQ: This should become an assert again...

As this isn't a code quality issue and the two cases where this can happen are explained below.

CarolEidt · 2018-11-07T23:04:55Z

tests/src/JIT/Regression/JitBlue/GitHub_18259/GitHub_18259.cs

+
+using System;
+
+// Test case for issues with a missing check for ZeroOffsetFldSeq values on LclVar reads


A little more detail here and/or on the specific cases would be informative for future reference.

Without the fix several cases fail because value numbering gives us the old value instead of the modified value.
The original bug was submitted by the Fuzzlyn C# generator program.
(see issue #18259)

I wrote up this test case to cover alot of the different cases.
Without my fix this test case prints:

D:\Public\Bugs\GitHub_18259>CoreRun.exe GitHub_18259.exe
Failed - Test_e0_S2_F3_F1() - 640
Failed - Test_e0_S2_F4_F1() - 960
Failed - Test_e1_S2_F3_F1() - 640
Failed - Test_e1_S2_F4_F1() - 960
Failed

I also have a smaller test case that I can add as well.

src/jit/optimizer.cpp

CarolEidt · 2018-11-07T23:11:01Z

src/jit/valuenum.cpp

+                        if (addrExtended != ValueNumStore::NoVN)
+                        {
+                            lcl->gtVNPair.SetBoth(addrExtended);
+                        }


If addrExtended was NoVN, is it correct that we neither have generateUniqueVN set to true, nor have we set lcl->gtVNPair?

I orginally did try that out (setting generateUniqueVN) but it turs out that doing that caused a few codegen regressions.

So - what value number will lcl get in that case?

The code above dealing with wholeLclVarVNP will either assign
lcl->gtVNPair = wholeLclVarVNP;
or
lcl->gtVNPair = partialLclVarVNP;
or
set generateUniqueVN = true;

For the code regression case we had (typSize == varSize) so we assigned wholeLclVarVNP
and this allowed the address of a struct byref to be CSE-ed with the address of a (struct byref + zero-offset field)

sandreenko · 2018-11-08T00:23:37Z

src/jit/valuenum.cpp

@@ -3662,24 +3661,47 @@ ValueNum ValueNumStore::VNApplySelectorsAssign(
        }

        // Otherwise, fldHnd is a real field handle.
-        CORINFO_FIELD_HANDLE fldHnd = fieldSeq->m_fieldHnd;
+        CORINFO_FIELD_HANDLE fldHnd    = fieldSeq->m_fieldHnd;
+        CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE;


I can't find where is the actual fix and where is refactoring.
I do not see any uses of structHnd, why do we need this variable?

I will remove this unused variable

AndyAyersMS

Would appreciate it if the main commit for the fix contained a bit more detail about what the bug was and why it lead to bad codegen (and whether it was a regression and if so when/how..., and if not why we seemingly overlooked this case before or we just never saw it for some reason). Or if not suitable for the commit, at least put this in the initial note to reviewers.

I realize you can go back to the original bug and dig some of this out or reverse parse it from the new tests but it takes a while. Also the initial part of an issue usually contains a lot of guesswork.

The fix looks plausible but I wonder if there are other cases like this lurking about.

src/jit/optimizer.cpp

The problem here was that we have an indirection of a LclVar that was a pointer to an array element whose type is a struct. The following discussion refers to the test case GitHub_18259/GitHub_18259.cs We recorded that the struct field F1.F0 is assigned 1234u. With the bug all subsequent reads of vr7.F0 return this value. We miss the update to zero because we didn't add the Zero Field sequence value to the LclVar vr7 Added Test case GitHub_18259.cs Added Test case GitHub_20838.cs

briansull · 2018-11-08T19:35:50Z

Additional small test case added
main commit message updated: a35f062

briansull · 2018-11-08T22:18:10Z

@AndyAyersMS PTAL
Is there anything more you want for approval.

I do plan on investigating this area for adfditional work.
I also plan on adding some ConfigValues to allow us to disable some of the more esoteric areas of VN.

AndyAyersMS

Yeah I think this is ok.

Those zero offset cases always seem to be troublemakers. I wonder if we should try and rework how they're implemented to make them look like nonzero offset accesses, at least early on in the jit.

ValueNum add check for ZeroOffsetFldSeq on LclVar reads Commit migrated from dotnet/coreclr@ae4dc9c

CarolEidt reviewed Nov 7, 2018

View reviewed changes

sandreenko reviewed Nov 8, 2018

View reviewed changes

AndyAyersMS reviewed Nov 8, 2018

View reviewed changes

src/jit/optimizer.cpp Show resolved Hide resolved

briansull force-pushed the issue_18259 branch from 81f8f91 to a35f062 Compare November 8, 2018 19:34

AndyAyersMS approved these changes Nov 8, 2018

View reviewed changes

briansull merged commit ae4dc9c into dotnet:master Nov 8, 2018

briansull deleted the issue_18259 branch November 8, 2018 23:05

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/coreclr#20838 from briansull/issue_18259

d930074

ValueNum add check for ZeroOffsetFldSeq on LclVar reads Commit migrated from dotnet/coreclr@ae4dc9c

SingleAccretion mentioned this pull request Apr 30, 2022

Physical value numbering dotnet/runtime#68712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ValueNum add check for ZeroOffsetFldSeq on LclVar reads #20838

ValueNum add check for ZeroOffsetFldSeq on LclVar reads #20838

briansull commented Nov 6, 2018

briansull commented Nov 7, 2018 •

edited

Loading

briansull commented Nov 7, 2018

briansull commented Nov 7, 2018

briansull commented Nov 7, 2018

CarolEidt left a comment

CarolEidt Nov 7, 2018

briansull Nov 7, 2018 •

edited

Loading

briansull Nov 8, 2018 •

edited

Loading

CarolEidt Nov 7, 2018

briansull Nov 7, 2018 •

edited

Loading

briansull Nov 8, 2018

CarolEidt Nov 7, 2018

briansull Nov 7, 2018

CarolEidt Nov 8, 2018

briansull Nov 8, 2018 •

edited

Loading

sandreenko Nov 8, 2018

briansull Nov 8, 2018

AndyAyersMS left a comment

briansull commented Nov 8, 2018

briansull commented Nov 8, 2018

AndyAyersMS left a comment


		using System;

		// Test case for issues with a missing check for ZeroOffsetFldSeq values on LclVar reads

ValueNum add check for ZeroOffsetFldSeq on LclVar reads #20838

ValueNum add check for ZeroOffsetFldSeq on LclVar reads #20838

Conversation

briansull commented Nov 6, 2018

briansull commented Nov 7, 2018 • edited Loading

briansull commented Nov 7, 2018

briansull commented Nov 7, 2018

briansull commented Nov 7, 2018

CarolEidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansull Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

briansull Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansull Nov 7, 2018 • edited Loading

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

briansull Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

briansull commented Nov 8, 2018

briansull commented Nov 8, 2018

AndyAyersMS left a comment

Choose a reason for hiding this comment

briansull commented Nov 7, 2018 •

edited

Loading

briansull Nov 7, 2018 •

edited

Loading

briansull Nov 8, 2018 •

edited

Loading

briansull Nov 7, 2018 •

edited

Loading

briansull Nov 8, 2018 •

edited

Loading