JIT: Fix reordering of call args on x86

[jakobbotsch]

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]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

---

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.

Author:	jakobbotsch
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[jakobbotsch]

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.

[jakobbotsch]

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.

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

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

[jakobbotsch]

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
x86 output before this change: Before

This kind of reordering is the cause of the majority of the diffs in benchmarks: a single method (MessagePack.MessagePackBinary.cctor) had extensive reordering of this form before this change.