Fix VNs for method table fetched from newobj

[jkotas]

NativeAOT compiler creates different type symbols for constructed (e.g. new MyType) and other uses (e.g. typeof(MyType)). It allows it to omit full vtable when the type is never constructed e.g. when the type is only used as typeof(MyType). These symbols are folded together near final linking phase.

Optimization introduced in dotnet/runtime#52476 will make assertion prop compare the symbols, assuming that two different symbols will have two different addresses. It is not the case for constructed and unconstructed types since they are folded together later.

[jkotas]

Temp workaround added in #1113

[jkotas]

A few options that I can think of:

• Option 1: Use CORINFO_CLASS_HANDLEs for VNs identity instead of symbols
• Option 2: Add JIT/EE interface to compare symbols
• Option 3: Do not assume in assertion prop that VNs of different symbols are guaranteed to be different values at runtimes
  I am not sure which one of these options would work best.

@AndyAyersMS Do you have thoughts about the best way to fix this?

[AndyAyersMS]

In option 1 you're saying there is a common type model artifact that both symbols map to? If so, we could update ValueNumStore::VNForHandle to do this mapping when the jit is running against native AOT.

There's a danger that we might try and rematerialize the constant out of the VN back into the IR as a handle. I can't say for sure if this happens or doesn't happen. So we might need to keep the backing symbol as well, or keep icon flags that we could use to either block this rematerialization or else call back through the jit interface with the flags and artifact to recover the symbol. Or just keep the symbol with the VNHandle data too (there likely aren't that many of these, so an extra field might be ok).

For option 2 we already have compareTypesForEquality on the jit interface but to fix VN to use this we'd need to find all the places in VN where we assume for constants that different VNs imply different constants. That might be hard. Likewise we make this same sort of assumption for option 3.

Other places in the jit that make deductions based on class handle inequality could be updated to use compareTypesForEquality though -- not sure if there are any of these places.

So I guess I'd go with option 1 in VN, and option 2 (using the existing method) outside of VN.

[MichalStrehovsky]

I'm trying an alternative approach to fixing this - in optimized builds we run a scanner before RyuJIT compilation happens so we have a pretty good idea of what types will need each form of the symbol. We can use that info to provide a consistent view for RyuJIT (one symbol == one type) and don't need to do the shenanigans with different symbol for casts vs newobj.

In trying that approach, I'm running into an issue (looks value numbering related) where RyuJIT seems to be forgetting that one of the constants it's working with is a handle and forgets to generate relocs for it.

I'll look more into it tomorrow but since the day didn't start yet in Redmond, @AndyAyersMS would you be able to give me some pointers where to look?

Here's the Ngendump:
jitdump.txt

The interesting number to look for is 420088 that starts out as a handle, but shows up as "LngCns" in valuenumbering (I assume it should be "Hnd const" like the constant for the newobj before that), and eventually ends up being generated into:

IN0004: 000014 mov      rax, 0x4000000000420088
IN0005: 00001E call     [rax]Base:GetFoo():BringUpTest+TestCovariantReturns+IFoo:this

That segfaults at runtime.

[AndyAyersMS]

I am off work for a few days.

Maybe @jakobbotsch or @SingleAccretion can help?

[SingleAccretion]

@MichalStrehovsky This is a morph bug, fgExpandVirtualVtableCallTarget is creating invalid IR with ADDs of longs with ints. This "confuses" VN to go down a path where we don't account for handles.

N007 (  6,  5) [000076] n--X-------- control expr    \--*  IND       long
N006 (  4,  3) [000075] ---X---N----                    \--*  ADD       long
N004 (  3,  2) [000073] #--X--------                       +--*  IND       long
N003 (  1,  1) [000072] ------------                       |  \--*  LCL_VAR   ref    V02 tmp1         u:2 (last use)
N005 (  1,  1) [000074] ------------                       \--*  CNS_INT   int    24

P.S. Not sure exactly which path is hit in fgExpandVirtualVtableCallTarget, but both are "wrong".

[jkotas]

So the fix should be to change all gtNewIconNode(..., TYP_INT) in fgExpandVirtualVtableCallTarget to gtNewIconNode(..., TYP_INT_IMPL)?

[SingleAccretion]

I believe so.

[MichalStrehovsky]

That seems to do the trick, thank you!

There's still some other stuff that appears after uncommenting this block, so I'm dealing with that and no pull request yet.

[MichalStrehovsky]

Fixed in #1754.