Initialize gtUseNum before use #57256

kunalspathak · 2021-08-12T01:58:25Z

gtUseNum remained uninitialized until the use.

kunalspathak · 2021-08-12T01:58:32Z

@dotnet/jit-contrib

ghost · 2021-08-12T02:03:29Z

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 minutes, a condition that will be fulfilled in about 4 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

kunalspathak · 2021-08-12T16:37:53Z

linux arm failure is related to #55441

kunalspathak · 2021-08-12T16:38:26Z

I am seeing a new issue on OSX/arm64 that was discovered by the test I added.

Assert failure(PID 92709 [0x00016a25], Thread: 75045132 [0x479190c]): Assertion failed 'false && "it's not expected to be hit at the moment, but can be allowed."' in 'System.Runtime.CompilerServices.CastHelpers:ChkCastClassSpecial(long,System.Object):System.Object' during 'Do value numbering' (IL size 100)

briansull

Looks Good

kunalspathak · 2021-08-12T19:14:59Z

@briansull - We hit this code when we do multiple iterations of optimization.

runtime/src/coreclr/jit/valuenum.cpp

Lines 8418 to 8419 in 5f534eb

    
           assert(false && "it's not expected to be hit at the moment, but can be allowed."); 
        
           // tree->gtVNPair = vnStore->VNPWithExc(tree->gtVNPair, addrXvnp);

As seen below, in 1st iteration, we CSE the [000081]

***** BB01
STMT00021 (IL 0x007...  ???)
N004 (  6, 14)              [000095] -A--G---R---              *  ASG       ref    $c3
N003 (  1,  1)              [000094] D------N----              +--*  LCL_VAR   ref    V05 tmp2         d:1 $c3
N002 (  6, 14)              [000082] #---G-------              \--*  IND       ref    $c3
N001 (  3, 12) CSE #01 (def)[000081] H-----------                 \--*  CNS_INT(h) long   0xD9673020 [ICON_STR_HDL] $2c0

...
...

Considering CSE #01 {K_000001A1D9673000} [def=100.000000, use=50.000000, cost=  3      ]
CSE Expression : 
N001 (  3, 12) CSE #01 (def)[000081] H-----------              *  CNS_INT(h) long   0xD9673020 [ICON_STR_HDL] $2c0

Aggressive CSE Promotion (250.000000 >= 200.000000)
cseRefCnt=250.000000, aggressiveRefCnt=200.000000, moderateRefCnt=100.000000
defCnt=100.000000, useCnt=50.000000, cost=3, size=12
def_cost=1, use_cost=1, extra_no_cost=22, extra_yes_cost=0
CSE cost savings check (172.000000 >= 150.000000) passes

Promoting CSE:

lvaGrabTemp returning 6 (V06 rat0) (a long lifetime temp) called for CSE - aggressive.
CSE #01 is single-def, so associated CSE temp V06 will be in SSA
New refCnts for V06: refCnt =  2, refCntWtd = 2   
New refCnts for V06: refCnt =  3, refCntWtd = 2.50

We have shared Const CSE's and selected $2c0 with a value of 0x000001A1D9673020 as the base.

CSE #01 def at [000081] replaced in BB01 with def of V06
optValnumCSE morphed tree:
N008 ( 13, 19)              [000095] -A--G---R---              *  ASG       ref    $c3
N007 (  1,  1)              [000094] D------N----              +--*  LCL_VAR   ref    V05 tmp2         d:1 $c3
N006 ( 13, 19)              [000082] #A--G-------              \--*  IND       ref    $c3
N005 ( 10, 17)              [000131] -A----------                 \--*  COMMA     long   $2c0
N003 (  7, 15)              [000129] -A------R---                    +--*  ASG       long   $VN.Void
N002 (  3,  2)              [000128] D------N----                    |  +--*  LCL_VAR   long   V06 cse0         d:1 $2c0
N001 (  3, 12)              [000081] H-----------                    |  \--*  CNS_INT(h) long   0xD9673020 [ICON_STR_HDL] $2c0
N004 (  3,  2)              [000130] ------------                    \--*  LCL_VAR   long   V06 cse0         u:1 $2c0

During 2nd iteration (because COMPlus_JitOptRepeat=ChkCastClassSpecial), we try to value number the COMMA node but then hit the assert:

***** BB01, STMT00021(before)
N008 ( 13, 19) [000095] -A--G---R---              *  ASG       ref   
N007 (  1,  1) [000094] D------N----              +--*  LCL_VAR   ref    V05 tmp2         d:1
N006 ( 13, 19) [000082] #A--G-------              \--*  IND       ref   
N005 ( 10, 17) [000131] -A----------                 \--*  COMMA     long  
N003 (  7, 15) [000129] -A------R---                    +--*  ASG       long  
N002 (  3,  2) [000128] D------N----                    |  +--*  LCL_VAR   long   V06 cse0         d:1
N001 (  3, 12) [000081] H-----------                    |  \--*  CNS_INT(h) long   0xD9673020 [ICON_STR_HDL]
N004 (  3,  2) [000130] ------------                    \--*  LCL_VAR   long   V06 cse0         u:1

The assert message says that it is fine to have different operator, but wanted to check with you what should be the desired behavior?

briansull · 2021-08-12T19:42:28Z

@kunalspathak - That assert was added by EgorBo on Mar 22 with PR #50000

https://github.com/dotnet/runtime/pull/50000/files#r598977718

We have replaced the CNS_INT with the COMMA tree introduced during CSE.

kunalspathak · 2021-08-12T22:29:53Z

Thanks @briansull . @EgorBo - any thoughts?

EgorBo · 2021-08-13T16:19:52Z

@kunalspathak I think it should be safe to change this line

runtime/src/coreclr/jit/valuenum.cpp

Line 8411 in 60d9b98

if (addr->IsCnsIntOrI())

to be

f (addr->gtEffectiveVal()->IsCnsIntOrI())

or maybe even here

runtime/src/coreclr/jit/valuenum.cpp

Line 8370 in 60d9b98

GenTree* addr = tree->AsIndir()->Addr();

🤔

briansull · 2021-08-13T16:47:13Z

@kunalspathak I think it should be safe to change this line
to be
f (addr->gtEffectiveVal()->IsCnsIntOrI())

The gtEffectiveVal call will just pull out the CSE LclVar that gets assigned to be the constant.
Then IsCnsIntOrI will still be false.

EgorBo · 2021-08-13T16:51:50Z

@kunalspathak I think it should be safe to change this line
to be
f (addr->gtEffectiveVal()->IsCnsIntOrI())

The gtEffectiveVal call will just pull out the CSE LclVar that gets assigned to be the constant.
Then IsCnsIntOrI will still be false.

Ah, indeed, then it's ->gtCommaAssignVal()

EgorBo · 2021-08-13T16:54:16Z

As the assert states: it can be just removed - there should not be a case where GT_IND has GTF_IND_NONNULL but the address was changed to something that can be null...

briansull · 2021-08-13T17:08:14Z

You could assert that there is no exception set on the gtEffectiveVal, (which requires a new call to unpack)
and/or check to see if the gtEffectiveVal ValueNumber is still a constant.

kunalspathak · 2021-08-13T21:51:56Z

You could assert that there is no exception set on the gtEffectiveVal, (which requires a new call to unpack)
and/or check to see if the gtEffectiveVal ValueNumber is still a constant.

Did I get it correct?

assert(addr->gtEffectiveVal()->IsCnsIntOrI() || addr->gtEffectiveVal()->gtVNPair.GetLiberal() == ValueNumStore::VNForEmptyExcSet());

briansull · 2021-08-14T00:37:02Z

addr->gtEffectiveVal()->gtVNPair.GetLiberal()

No, an unpack operation is necessary, you can't do it without introducing new DEBUG only variables.
Example:

            // Unpack, Norm,Exc for 'rhsVNPair'
            ValueNum vnRhsLibNorm;
            vnStore->VNUnpackExc(rhsVNPair.GetLiberal(), &vnRhsLibNorm, &vnExcSet);

Also the changes for this PR doesn't include any changes to ValueNumber.cpp
So I am not seeing your code changes (other than in the comment above)

kunalspathak · 2021-08-14T01:08:37Z

Also the changes for this PR doesn't include any changes to ValueNumber.cpp

Right, I wanted to make sure that I get it right. So what you are recommending is:

#ifdef DEBUG

                            ValueNum effectiveVnExcSet;
                            ValueNum effectiveValueNum;
                            vnStore->VNUnpackExc(addr->gtEffectiveVal()->gtVNPair.GetLiberal(), &effectiveValueNum,
                                                 &effectiveVnExcSet);
                            assert(addr->gtEffectiveVal()->IsCnsIntOrI() ||
                                   effectiveVnExcSet == ValueNumStore::VNForEmptyExcSet());

#endif
                            tree->gtVNPair = vnStore->VNPWithExc(tree->gtVNPair, addrXvnp);

SingleAccretion · 2021-08-14T18:53:49Z

No, an unpack operation is necessary

Hmm, I though you could also use the VNHasExc helper method in such situations. Is that not true?

EgorBo · 2021-08-16T15:25:55Z

Also the changes for this PR doesn't include any changes to ValueNumber.cpp
So I am not seeing your code changes (other than in the comment above)

From my understanding that assert can be hit in Main when you enable CSE for constants (string literal handle in this case).

So my suggestion is to rewrite it like this: https://www.diffchecker.com/n2HaLLdk

kunalspathak · 2021-08-16T18:22:47Z

I have opened #57515 and disabled the test for Arm64 for now.

kunalspathak · 2021-08-16T21:21:39Z

The failure is related to #57452.

kunalspathak · 2021-08-17T04:35:36Z

Failures are unrelated.

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

AndyAyersMS approved these changes Aug 12, 2021

View reviewed changes

kunalspathak added the auto-merge label Aug 12, 2021

jakobbotsch approved these changes Aug 12, 2021

View reviewed changes

briansull approved these changes Aug 12, 2021

View reviewed changes

kunalspathak mentioned this pull request Aug 16, 2021

Non-null indirection should consider COMMA (CNS_INT) nodes #57515

Closed

kunalspathak added 4 commits August 16, 2021 12:34

Initialize gtUseNum

9c644c3

Add test case

d80c22e

catch the exceptions in test case

ff5a6b2

kind of disable test

61ccb10

kunalspathak force-pushed the gtUseNum branch from 82305a3 to 61ccb10 Compare August 16, 2021 19:38

try to exclude the test one more time

96e5cfb

kunalspathak merged commit 53d3281 into dotnet:main Aug 17, 2021

This was referenced Aug 17, 2021

Feed unreliability affecting CI #55449

Closed

Testing out the new system jaredpar/runfo#71

Open

runfoapp bot mentioned this pull request Aug 17, 2021

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

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize gtUseNum before use #57256

Initialize gtUseNum before use #57256

kunalspathak commented Aug 12, 2021

kunalspathak commented Aug 12, 2021

ghost commented Aug 12, 2021

kunalspathak commented Aug 12, 2021

kunalspathak commented Aug 12, 2021

briansull left a comment

kunalspathak commented Aug 12, 2021

briansull commented Aug 12, 2021 •

edited

Loading

kunalspathak commented Aug 12, 2021

EgorBo commented Aug 13, 2021 •

edited

Loading

briansull commented Aug 13, 2021

EgorBo commented Aug 13, 2021

EgorBo commented Aug 13, 2021

briansull commented Aug 13, 2021 •

edited

Loading

kunalspathak commented Aug 13, 2021

briansull commented Aug 14, 2021

kunalspathak commented Aug 14, 2021

SingleAccretion commented Aug 14, 2021

EgorBo commented Aug 16, 2021

kunalspathak commented Aug 16, 2021

kunalspathak commented Aug 16, 2021

kunalspathak commented Aug 17, 2021

Initialize gtUseNum before use #57256

Initialize gtUseNum before use #57256

Conversation

kunalspathak commented Aug 12, 2021

kunalspathak commented Aug 12, 2021

ghost commented Aug 12, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

kunalspathak commented Aug 12, 2021

kunalspathak commented Aug 12, 2021

briansull left a comment

Choose a reason for hiding this comment

kunalspathak commented Aug 12, 2021

briansull commented Aug 12, 2021 • edited Loading

kunalspathak commented Aug 12, 2021

EgorBo commented Aug 13, 2021 • edited Loading

briansull commented Aug 13, 2021

EgorBo commented Aug 13, 2021

EgorBo commented Aug 13, 2021

briansull commented Aug 13, 2021 • edited Loading

kunalspathak commented Aug 13, 2021

briansull commented Aug 14, 2021

kunalspathak commented Aug 14, 2021

SingleAccretion commented Aug 14, 2021

EgorBo commented Aug 16, 2021

kunalspathak commented Aug 16, 2021

kunalspathak commented Aug 16, 2021

kunalspathak commented Aug 17, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

briansull commented Aug 12, 2021 •

edited

Loading

EgorBo commented Aug 13, 2021 •

edited

Loading

briansull commented Aug 13, 2021 •

edited

Loading