Question about GetFoldedArithOpResultHandleFlags()

[BruceForstall]

#70452 introduced GetFoldedArithOpResultHandleFlags() that loses specific handle types and seems to associate incorrect handle types with arithmetic operations on handles. This problem is seen during OptRepeat compilations with shared constant CSE.

Consider:

This tree defines V134 tmp133 as a base constant for shared constant CSE:

STMT00003 ( 0x007[--] ... ??? )
N009 ( 41, 42) [000008] -ACXG-------                        *  CALL      void   <unknown method> $VN.Void
N006 ( 19, 20) [002663] DAC--------- arg1 setup             +--*  STORE_LCL_VAR ref    V122 tmp121      d:2 $VN.Void
N005 ( 19, 20) [000003] -AC---------                        |  \--*  CALL help ref    CORINFO_HELP_NEWSFAST $1c0
N004 (  3,  9) [002869] -A---------- arg0 in r0             |     \--*  COMMA     int    $140
N002 (  2,  8) [002867] DA----------                        |        +--*  STORE_LCL_VAR int    V134 tmp133      d:2 $VN.Void
N001 (  2,  8) [000002] H-----------                        |        |  \--*  CNS_INT(h) int    -0x5275842C class <unknown class> $140
N003 (  1,  1) [002868] ------------                        |        \--*  LCL_VAR   int    V134 tmp133      u:2 $140
N007 (  1,  1) [002664] ------------ arg1 in r1             +--*  LCL_VAR   ref    V122 tmp121      u:2 (last use) $1c0
N008 (  2,  8) [000009] H----------- gctx in r0             \--*  CNS_INT(h) int    -0x52758314 method <unknown method> $141

This tree defines V38 tmp37 as an offset from V134 tmp133:

STMT00118 ( 0x00C[E-] ... ??? )
N008 ( 22, 26) [000628] DACXG-------                        *  STORE_LCL_VAR byref  V38 tmp37        d:2 $1c2
N007 ( 22, 26) [000015] --CXG-------                        \--*  COMMA     int    $242
N003 ( 19, 22) [000014] H-CXG-------                           +--*  CALL help int    CORINFO_HELP_CLASSINIT_SHARED_DYNAMICCLASS $241
N001 (  2,  8) [000012] ------------ arg0 in r0                |  +--*  CNS_INT   int    -0x4BFB5600 $4f
N002 (  1,  2) [000013] ------------ arg1 in r1                |  \--*  CNS_INT   int    1 $50
N006 (  3,  4) [002872] -------N----                           \--*  ADD       int    $142
N004 (  1,  1) [002870] ------------                              +--*  LCL_VAR   int    V134 tmp133      u:2 $140
N005 (  1,  2) [002871] ------------                              \--*  CNS_INT   int    492 $51

Node [000624] uses V38 tmp37. Note in particular the IND is NOT invariant:

STMT00149 ( INL10 @ 0x000[E-] ... ??? ) <- INL03 @ 0x051[E-] <- INLRT @ 0x00C[E-]
N005 (  7,  7) [000729] ----GO------                        *  JTRUE     void   $VN.Void
N004 (  5,  5) [000728] J---GO-N--S-                        \--*  EQ        int    <l:$261, c:$262>
N002 (  3,  2) [000726] n---GO------                           +--*  IND       int    <l:$286, c:$93>
N001 (  1,  1) [000624] ------------                           |  \--*  LCL_VAR   byref  V38 tmp37        u:2 $142
N003 (  1,  2) [000727] ------------                           \--*  CNS_INT   int    11 $52

After VN-based fold of [000624]:

STMT00149 ( INL10 @ 0x000[E-] ... ??? ) <- INL03 @ 0x051[E-] <- INLRT @ 0x00C[E-]
N005 (  7,  7) [000729] ----GO------                        *  JTRUE     void   $VN.Void
N004 (  5,  5) [000728] J---GO-N--S-                        \--*  EQ        int    <l:$261, c:$262>
N002 (  3,  2) [000726] #---GO------                           +--*  IND       int    <l:$286, c:$93>
               [003090] H-----------                           |  \--*  CNS_INT(h) int    -0x52758240 const ptr $142
N003 (  1,  2) [000727] ------------                           \--*  CNS_INT   int    11 $52

Note in particular that the replacement has const ptr handle type, and because of that, the IND is marked invariant (# flag). The computed constant starts as GTF_ICON_CLASS_HDL, the handle type of the constant assigned to V134 tmp133. With shared constant CSE, we add 492 to get the actual value, which originally was:

N004 (  2,  8) CSE #01 (use)[000010] H-----------                           \--*  CNS_INT(h) int    -0x52758240 static Fseq[<unknown field>] $142

Then GetFoldedArithOpResultHandleFlags maps GTF_ICON_CLASS_HDL to GTF_ICON_CONST_PTR.

Of course, this is wrong. But also, because it is "constant" and the IND marked invariant, that means the IND can later be CSE'd, incorrectly.

Question: wouldn't it be safer, in the presence of shared constant CSE, to always fold handle types to (mutable) GTF_ICON_GLOBAL_PTR? Or lose the handle-ness of them entirely?

For instance, what does it mean for unary op neg/not/bswap16/bswap on a handle constant? How can the result be a handle? And what does it mean to add some arbitrary number N to a class handle? Are there cases where it makes sense?

@jakobbotsch @AndyAyersMS @EgorBo @SingleAccretion

[dotnet-policy-service]

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

[jakobbotsch]

Question: wouldn't it be safer, in the presence of shared constant CSE, to always fold handle types to (mutable) GTF_ICON_GLOBAL_PTR? Or lose the handle-ness of them entirely?

Losing the handle-ness seems ok to me in runtime JIT. Is it also ok during AOT?

And what does it mean to add some arbitrary number N to a class handle? Are there cases where it makes sense?

Yes, when accessing fields off of MethodTable or when indexing into RVA statics we see these patterns. IIRC those are the cases I hit in #70452.

I don't see how #70452 is related to the issue at hand. The problem exists before that PR. Instead of mapping the handle resulting of the addition from GTF_ICON_CLASS_HDL to GTF_ICON_CONST_PTR, the previous logic would have kept it as GTF_ICON_CLASS_HDL, "even more" wrong.

As a general note: we keep running into problems around these handles and their representation. Most of the problems seem to be rooted in the fact that we need to treat some handles opaquely during prejit. I think we should introduce a separate node, e.g. GT_HANDLE or similar, that we can utilize during AOT only and that can be handled separately as required.

[SingleAccretion]

#100060 is the right fix for this problem. It was never correct to "derive" handle kinds from the inputs, we just haven't run into problems because the handle kinds on VNs are sparsely used.

#100060 will also fix the problem seen in #99011 (comment).

[MichalPetryka]

#100060 will also fix the problem seen in #99011 (comment).

I'll verify that after it's merged FYI (I have a reliable repro for it (that works even without JitOptRepeat) locally which however doesn't seem to fire on the CI for some reason, probably something makes the assert not reachable on arm32).

[BruceForstall]

Losing the handle-ness seems ok to me in runtime JIT. Is it also ok during AOT?

During AOT compReloc will be true and we won't create shared constant CSE out of handles, nor will we allow VN folding of handles.

I don't see how #70452 is related to the issue at hand.

True, it was a problem beforehand. I just found this function/PR when I noticed the mapping causing unexpected behavior (before, the unexpected handle type still would have appeared).

[BruceForstall]

Fixed by #100060

[MichalPetryka]

#100060 will also fix the problem seen in #99011 (comment).

I'll verify that after it's merged FYI (I have a reliable repro for it (that works even without JitOptRepeat) locally which however doesn't seem to fire on the CI for some reason, probably something makes the assert not reachable on arm32).

Can confirm the assert in my sample is gone now.