-
Notifications
You must be signed in to change notification settings - Fork 2.7k
JIT: Allow struct promotion in GS frames #16220
JIT: Allow struct promotion in GS frames #16220
Conversation
Enable struct promotion in methods where the jit is also emtting a stack guard. Not allowing promotion seems like an unnecessary limitation and leads to poor performance in methods with both Span and stackalloc. Closes #16197.
Just getting some broader testing. In case anyone's interested, diffs look good:
Some crazy improvements in methods that use both Fx code improvements not showing up in jit-diffs; likely hampered by R2R codegen inhibiting cross-assembly inlines of |
@BruceForstall @jashook what's the magic needed to invoke pri-1 tests these days? |
Will also do some testing over on desktop. |
Check @jashook @RussKeldorph fyi, it looks like some of the jobs are named weirdly with a trailing regex: |
Welcome to the dotnet/coreclr Perf help The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:master. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:master. Click to expand
Have a nice day! |
@dotnet-bot test Windows_NT x64 Checked corefx_baseline |
corefx jobs broken once again:
will have to retry once things are back in sync. |
In the zap disable tests the "eventpipesmoke" test fails when zap is disabled. The test seems to run ok but the output validation fails; likely it is anticipating a large set of rundown events from the corelib ngen image that never happen because we've disabled zap. So I'll disregard these failures. cc @nategraf. Not sure if you really need to do anything as we don't run this mode very often. There might be an exclusion mechanism for this test mode. Or you might see if |
We should make these tests succeed all the time, and if that means altering the test, or somehow disabling for ZapDisable, we need to do that. Regarding the statement "as we don't run this mode very often", that is not true: they are run regularly on a schedule, e.g. https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_windows_nt_zapdisable/, and you can see they started failing regularly with the introduction of the event test. cc @brianrob |
I'll look at the test with ZapDisable. Having a much lower success threshold for the amount of outputted will likely be the answer |
@nategraf if there is an easy way to modify this test then that's fine and let's do it. Otherwise, I think we'll need a way to opt-out of some of these more specific stress runs since we do need to be able to validate these types of scenarios and the tests need to have knowledge of the runtime configuration to be able to accurately detect that the right data is present. |
@nategraf sounds good. Thanks. |
Desktop testing going well so far. I will have to restart some of it to pick up the fix from #16235. Diffs look similar to the above. |
Desktop testing didn't uncover any issues. Still would like to get a CoreFX run before merging but it looks like things are still broken there. @briansull PTAL |
Rerunning now that these are fixed... @dotnet-bot test Windows_NT x86 Checked zapdisable |
CoreFX maybe working again...? @dotnet-bot test Windows_NT x64 Checked corefx_baseline |
Ubuntu CoreFx baseline failures seem common to many legs. So I think we're set for testing on this change. |
Ubuntu error is assertion failure https://github.com/dotnet/coreclr/issues/16331 |
@dotnet/jit-contrib PTAL, this is ready for review.... |
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. Thanks for the added JITDUMP
messages.
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.
Looks Good
Enable struct promotion in methods where the jit is also emtting a stack guard.
Not allowing promotion seems like an unnecessary limitation and leads to poor
performance in methods with both Span and stackalloc.
Closes #16197.