JIT: teach VN to fold type comparisons #72136
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsTeach VN to fold some comparisons involving calls to Closes #71909.
|
|
cc @dotnet/jit-contrib Could defer this until RC snap. Not sure how strict we're going to be. |
| // Value number counterpart to gtFoldTypeCompare | ||
| // Doesn't handle all the cases (yet). | ||
| // | ||
| // (EQ/NE (TypeHandleToRuntimeType x) (TypeHandleToRuntimeType y)) == (EQ/NE x y) |
There was a problem hiding this comment.
I don't think this function is injective, e.g. all function pointers map to typeof(IntPtr).
There was a problem hiding this comment.
I suppose we should check with the runtime via compareTypesForEquality.
There was a problem hiding this comment.
I don't think this function is injective, e.g. all function pointers map to
typeof(IntPtr).
Worth to note that all function pointer typeofs currently compare as equal.
|
Seems like the jit gets this case wrong even without this new optimization: typeof(delegate*<int, double>) == typeof(delegate* unmanaged<float, void*, void>);we fold this in the importer when optimizing: Seems like the jit interface methods |
|
|
I'd check that it does not break any of these: using System.Runtime.CompilerServices;
public enum Enum1 : int { A }
public enum Enum2 : uint { A }
class Program
{
static void Main()
{
// false
Console.WriteLine(Test<int, Enum1>());
Console.WriteLine(Test<Enum1, Enum2>());
Console.WriteLine(Test<int?, uint?>());
Console.WriteLine(Test<int?, Enum1?>());
Console.WriteLine(Test<ValueTuple<Program>, ValueTuple<string>>()); // ValueTuple<_Canon> vs ValueTuple<_Canon>
// true
Console.WriteLine(Test<Program, Program>());
Console.WriteLine(Test<ValueTuple<Program>, ValueTuple<Program>>());
}
[MethodImpl(MethodImplOptions.NoInlining)]
static bool Test<T1, T2>()
{
Type t = typeof(T1);
Console.WriteLine();
return t == typeof(T2);
}
} |
I guess the problem might be more on the JIT side in assuming injectivity of this helper again. It makes sense to me that the VM says that those two type handles are not equal, and there may be other cases in the JIT where that is the desired behavior. Unfortunately that means we may need a new JIT-EE method for this purpose... Although it looks like this function pointers are the only case of this, so maybe we can do something simple. |
The only broken ones I've found so far are function pointers.
(ignoring this PR for the moment) the jit only folds to class handle compares if the VM says it's ok to do so. So still think the fix needs to be done there... Also not sure what the rules are, for instance seems to be For this PR we need VN to call that same helper, and can only optimize the cases where we have known class handles. I still need to check the |
|
Not sure what the right fix is. We don't want to abandon the general idea that class handles uniquely map to types. But by the time the jit sees these types they're no longer function pointers, they are mapped to variants of Added a test case showing that we mis-optimize some cases (True0, False16) involving function pointer types when we optimize (even without the VN aspect added here). |
|
See also #11354 |
|
nits - as in the comments above, they are mapped to This reminds me of the old stuff where int vs uint and int vs MyEnumBasedOnInt were sometimes the same and sometimes different depending on isinst vs castclass vs unbox vs array cast vs probably more. I don't know if that's still the case, though I'm guessing yes given Egor's other example above. We had to have multiple sets of type rules depending on what question was being asked. (Indeed, if you construct an array of one of these delegate types and check if it is an array of the other, then they are kept separate.)
This sounds scary since this JIT doesn't own the mapping. But I guess if it's close enough (as suggested above, maybe it's only function pointers?), then the logic ("uniquely map unless function pointers") could be safely duplicated in the JIT, perhaps with an assertion/test/something to catch it if it changes in the future. I wonder if there are other corner cases though. Maybe specifying int vs System.Int32 in the encoding (or other weird stuff with TypeSpec encodings). Or int[0..5] vs int[2..5], which (if I recall correctly and hasn't changed from long ago) could be written in the IL types but didn't actually have semantic meaning. |
This turned out to be operator error; I was modifying the runtime but then only rebuilding the jit. They are indeed function pointer type descs.
Right -- but for some reason when the jit asks the runtime for the name of the type it gets back At any rate, since the handles are Presumably something similar will be needed for the jit interface methods over in crossgen2. |
Fascinating. I wonder if this is tied to the "evaluation stack" view of the world, where signed vs unsigned doesn't exist until you get to how specific instructions interpret the bits. (I, 12.3.2.1) |
|
As Jan noted elsewhere, Mono doesn't do things the same way. I'm also seeing some arm64 failures on coreclr that will need investigating. |
Or you can add JIT/EE interface method for this. The EE side has to have this mapping already. |
I created a map on the jit side for VN for now. There are some cases we will need to look at further, where the jit creates method table constants (eg via GDV) from "thin air" and so we have embedded handles with no associated compile time handle. Seems like to make this work for AOT we'll want to use the EE side map to figure out the right representation. |
|
Mono LLVM AOT failing with Opened #72460, will exclude for now. |
|
Installer failure is some kind of contention/timing issue Libraries test failure is #72429 SPMI shows about a 0.5% TP impact which is quite surprising. Going to investigate. |
|
I can reduce TP impact to around 0.1% with a bit of refactoring, and by revising the various Latter seems like it should be its own PR, so will just push the refactoring bit here. |
|
TP down to 0.2% but now, no SPMI diffs. Hmm. |
AndyAyersMS
force-pushed
the
VNFoldTypeCompare
branch
from
6f57790
to
ff07e51
Compare
|
Still seeing mono failures despite #72479. cc @vargaz TP is now 0.2% - 0.3%, still seems high for something that rarely kicks in. #72527 will help offset this somewhat. |
Teach VN to fold some comparisons involving calls to `TypeHandleToRuntimeType`. Closes dotnet#71909.
AndyAyersMS
force-pushed
the
VNFoldTypeCompare
branch
from
ff07e51
to
0c8cb31
Compare
|
@jakobbotsch PTAL |
Good question. There are diffs in the newly added test case, and there were SPMI diffs in earlier versions. It's possible that the SPMI queries made here now fail so the methods with diffs are failing in SPMI. Let me try running PMI diffs locally. |
|
PMI diffs |
Thanks. This seems like the kind of pattern that could be very prevalent in user code, so even with a low amount of hits in our code it seems reasonable to me to do it. Good example of a case where having a way to get diffs for user code would probably be beneficial. |
Teach VN to fold some comparisons involving calls to
TypeHandleToRuntimeType.Closes #71909.