Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VNs for method table fetched from newobj #1128

Closed
jkotas opened this issue May 15, 2021 · 10 comments
Closed

Fix VNs for method table fetched from newobj #1128

jkotas opened this issue May 15, 2021 · 10 comments
Labels
area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation

Comments

@jkotas
Copy link
Member

jkotas commented May 15, 2021

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 added a commit to jkotas/runtimelab that referenced this issue May 15, 2021
@jkotas jkotas added the area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation label May 15, 2021
@jkotas
Copy link
Member Author

jkotas commented May 15, 2021

Temp workaround added in #1113

@jkotas
Copy link
Member Author

jkotas commented May 15, 2021

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

I am off work for a few days.

Maybe @jakobbotsch or @SingleAccretion can help?

@SingleAccretion
Copy link

@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
Copy link
Member Author

jkotas commented Nov 25, 2021

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

@SingleAccretion
Copy link

I believe so.

@MichalStrehovsky
Copy link
Member

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
Copy link
Member

Fixed in #1754.

MichalStrehovsky added a commit to dotnet/runtime that referenced this issue Nov 30, 2021
Fixes issue observed in dotnet/runtimelab#1128 (comment) - the trees generate additions of `TYP_I_IMPL` with `TYP_INT` and that confuses things down the line.
jakobbotsch pushed a commit to dotnet/runtime that referenced this issue Dec 1, 2021
Fixes issue observed in dotnet/runtimelab#1128 (comment) - the trees generate additions of `TYP_I_IMPL` with `TYP_INT` and that confuses things down the line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr .NET runtime optimized for ahead of time compilation
Projects
None yet
Development

No branches or pull requests

4 participants