JIT: Fix reordering of call args on x86 #70931
Conversation
On x86 we skipped checking stack arguments for side effects with a comment that such arguments do not need to be evaluated into temps. While this is true a large part of the logic that follows is responsible for evaluating previous arguments into temps, and we must still do this.
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsOn x86 we skipped checking stack arguments for side effects with a
|
| #ifdef FEATURE_FIXED_OUT_ARGS | ||
| || arg.m_isTmp // Protect this by "FEATURE_FIXED_OUT_ARGS" to preserve the property | ||
| // that we only have late non-register args when that feature is on. | ||
| #endif |
There was a problem hiding this comment.
FEATURE_FIXED_OUT_ARGS is defined even on x86, just with value 0, so this #ifdef was always true. The point of this check is to catch when fgMakeOutgoingStructArgCopy has left a non-value node, and it only does this for FEATURE_FIXED_OUT_ARGS, so I have fixed the logic.
|
We will have test coverage for this once we loosen up the restrictions on forward subbing into function args. I expect we can do that once this and #70893 are merged. |
|
/azp run runtime-coreclr superpmi-diffs |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The regressions look more reasonable now after #70986: benchmarks.run.windows.x86.checked.mch:
Summary of Code Size diffs:
(Lower is better)
Total bytes of base: 8959310 (overridden on cmd)
Total bytes of diff: 8961437 (overridden on cmd)
Total bytes of delta: 2127 (0.02 % of base)
diff is a regression.
relative diff is a regression.
Detail diffs
coreclr_tests.pmi.windows.x86.checked.mch:
Summary of Code Size diffs:
(Lower is better)
Total bytes of base: 108583802 (overridden on cmd)
Total bytes of diff: 108588041 (overridden on cmd)
Total bytes of delta: 4239 (0.00 % of base)
diff is a regression.
relative diff is a regression.
Detail diffs
libraries.crossgen2.windows.x86.checked.mch:
Summary of Code Size diffs:
(Lower is better)
Total bytes of base: 26746067 (overridden on cmd)
Total bytes of diff: 26747642 (overridden on cmd)
Total bytes of delta: 1575 (0.01 % of base)
diff is a regression.
relative diff is a regression.
Detail diffs
libraries.pmi.windows.x86.checked.mch:
Summary of Code Size diffs:
(Lower is better)
Total bytes of base: 39964335 (overridden on cmd)
Total bytes of diff: 39965543 (overridden on cmd)
Total bytes of delta: 1208 (0.00 % of base)
diff is a regression.
relative diff is a regression.
Detail diffs
libraries_tests.pmi.windows.x86.checked.mch:
Summary of Code Size diffs:
(Lower is better)
Total bytes of base: 98150856 (overridden on cmd)
Total bytes of diff: 98162012 (overridden on cmd)
Total bytes of delta: 11156 (0.01 % of base)
diff is a regression.
relative diff is a regression.I've spot checked a few and they were cases where we were unsafely reordering things. In some cases we were reordering field accesses with static constructor initialization, which seems could cause observable behavior differences. In some other cases the reordering we did was ok but we do not have the necessary analysis to determine that it is ok (e.g. if stack arguments contained embedded assignments we could previously reorder them with register arguments without doing any checks, which is unsafe). cc @dotnet/jit-contrib PTAL @AndyAyersMS |
|
For example: using System;
public class Program
{
public static string[] Array = new string[1];
static Program()
{
Array[0] = Foo.Before; // Turns into CORINFO_HELP_ARRADDR_ST call, for which we reordered args on x86 before
}
public static void Main()
{
Console.WriteLine(Array[0]);
}
static class Foo
{
public static string Before = "Before";
static Foo()
{
Program.Array = new[] { "After" };
}
}
}non-x86 output: After This kind of reordering is the cause of the majority of the diffs in benchmarks: a single method ( |
2 participants
On x86 we skipped checking stack arguments for side effects with a
comment that such arguments do not need to be evaluated into temps.
While this is true a large part of the logic that follows is responsible
for evaluating previous arguments into temps, and we must still do this.