-
Notifications
You must be signed in to change notification settings - Fork 5.2k
JIT: remove guarded devirtualization restriction for small struct ret… #51157
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
JIT: remove guarded devirtualization restriction for small struct ret… #51157
Conversation
…urns Now that we're not retyping small structs, the existing transformation works for methods returning small structs too. Fixes dotnet#51138.
@sandreenko PTAL Will post diffs when I can. SPMI seems out of sync right now. |
// needed to fix up the return type. | ||
// | ||
// See for instance fgUpdateInlineReturnExpressionPlaceHolder. | ||
if (origCall->TypeGet() == TYP_STRUCT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was it protecting only from small struct returns and not from multi-reg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think it was just the small struct returns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that all struct calls were rejected here because of the one unsupported scenario(small structs). Now we don't need to do anything special for the previously unsupported case but we need to check that it works as expected for all of them.
I have ran the change locally with assert(false)
inside the block and we did not go into if (origCall->TypeGet() == TYP_STRUCT)
case on framework libraries crossgen/pmi x64/arm64 windows/unix. Is it expected?
However, I saw that we hit it aspnet.run.windows.x64
spmi collection but could not collect the diffs because when we delete this block we start hitting missing answers for the new questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This optimization requires PGO data, so not too surprising it doesn't fire much for libraries code.
Also by this point any struct return that uses a hidden buffer has been ntransformed and the return type is no longer TYP_STRUCT.
Here's a simple test case where the case does git hit. Run with COMPlus_TieredPGO=1
.
using System;
using System.Threading;
using System.Runtime.CompilerServices;
struct P
{
public int x;
public int y;
}
class B
{
virtual public P get() => new P();
}
class D : B
{
override public P get() => new P() { x = 67, y = 33 };
}
class X
{
public static int F(B b) => b.get().x + b.get().y;
[MethodImpl(MethodImplOptions.NoOptimization)]
public static int Main()
{
B b = new D();
int r = 0;
for (int i = 0; i < 100; i++)
{
r += F(b);
Thread.Sleep(15);
}
r = 0;
for (int i = 0; i < 10000; i++)
{
r += F(b);
}
r /= 10000;
Console.WriteLine($"r={r}");
return r;
}
}
I also ran jit-experimental CI below which gives us some coverage, and there were no unexpected failures (save for emptystacktrace\OOMException01 which seems to be failing everywhere).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Did some extra testing on SysV and verified GDV now handles 1 and 2 reg struct returns properly. |
…urns
Now that we're not retyping small structs, the existing transformation works
for methods returning small structs too.
Fixes #51138.