Use push for 8/12 byte struct args on x86

[SingleAccretion]

It is the case that currently for code like the following:

private static void Problem(StructWithIndex* p)
{
    CallForStructWithIndex(*p);
}

struct StructWithIndex
{
    public int Index;
    public int Value;
}

On x86 we generate the following:

       vmovq    xmm0, qword ptr [ecx]
       sub      esp, 8
       vmovq    qword ptr [esp], xmm0
       call     RyuJitReproduction.Program:CallForStructWithIndex(StructWithIndex)

While could generate:

       push     dword ptr [ecx+4]
       push     dword ptr [ecx]

Which is both a little faster (ah-hoc benchmarking on my machine shows ~10% improvement) and smaller. This change does just that, using push [addr] for 8 and 12 byte structs (the latter made the code simple).

To achieve that, some code refactoring was undertaken: we already had a path that did what we needed, but it was only used for structs with GC pointers. I deleted the double meaning of Kind::Push - for TYP_STRUCT it now means only one thing, that is genStructPutArgPush, while before this change it could also go down the "unroll" path (for the very specific cases of 8 and 12 byte structs that we are moving to using pushes).

As a cleanup item, PartialRepInstr kind was added, to represent the algorithm we use for storing structs with GC pointers on x64 Unix.

There will be two types of diffs with this change: first the one showed above, second - removal of unnecessary vzeroupper instructions that were being generated because LSRA was allocating an unnecessary XMM register for Kind::Push PUTARG_STKs with GC pointers.

Diffs - x86 only (no diffs on other targets as expected). All regressions are alignment artifacts.

[ghost]

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

Issue Details

---

Testing CI.

Highlight: benchmarks.run: Total bytes of delta: -11344 (-0.10 % of base).

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

[SingleAccretion]

Not sure what's up with SPMI in CI. Will assume the Windows x64 timeout is not related (it does not use PUTARG_STK).

@dotnet/jit-contrib some nice CQ improvements for x86 (as always, because I need them for another change).

[kunalspathak]

From unix-arm logs, seems some errors:

ERROR: Unexpected exception was thrown.

ERROR: Unexpected exception was thrown.

ERROR: Method 354 of size 7557059 failed to load and compile correctly by JIT2.

ERROR: main method 354 of size 7557059 failed to load and compile correctly.

ERROR: Couldn't load base metrics summary created by child process

Can you double check on your local machine?

For x86, they likely have timed out while producing .dasm files.

[kunalspathak]

second - removal of unnecessary vzeroupper instructions that were being generated because LSRA was allocating an unnecessary XMM register for Kind::Push PUTARG_STKs with GC pointers.

Could you paste the sample diff?

[kunalspathak]

Nice refactoring. Changes LGTM except the failure on superpmi linux-arm that you want to double check.

[SingleAccretion]

Can you double check on your local machine?

@kunalspathak Thank you for pointing that failure out, I did unfortunately miss it in the logs. It does not seem related: no ARM code paths were modified, and I can reproduce it both with and without this change.

It sure does look curious though: we're OOMing the Jit (2GB+ memory consumption and then an error in the CRT is hit).

Could you paste the sample diff?

They're all are like this:

        sub      esp, 20
-       vzeroupper
        xor      eax, eax

I suppose the better wording for this would have been that "LSRA was over-reporting the use of XMM registers".

[kunalspathak]

It sure does look curious though: we're OOMing the Jit (2GB+ memory consumption and then an error in the CRT is hit).

Where do you see that? in the logs?

[SingleAccretion]

Where do you see that? in the logs?

No, from locally replaying the coreclr_tests.pmi.Linux.arm.checked collection (no .mc because SPMI itself crashes).

[kunalspathak]

No, from locally replaying the coreclr_tests.pmi.Linux.arm.checked collection (no .mc because SPMI itself crashes).

Does it repro without your changes too?

[SingleAccretion]

As I note, yes.

Edit: I suppose in the CI log it also says both the baseline and diff Jits encountered the same error:

ERROR: Unexpected exception was thrown. <-- Base JIt

ERROR: Unexpected exception was thrown. <-- Diff Jit

ERROR: Method 354 of size 7557059 failed to load and compile correctly by JIT2. <-- Diff Jit

ERROR: main method 354 of size 7557059 failed to load and compile correctly. <-- Base Jit

[kunalspathak]

/azp run Fuzzlyn, Antigen

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[SingleAccretion]

Categorizing fuzzer failures.

It seems Fuzzlyn is timing out pretty consistently on Linux ARM, and we have one failure there:

Found example with seed 16498413861848824582 that hits error
JIT assert failed:
Assertion failed 'inVarToRegMap[varIndex] == REG_STK' in 'S1:M50(int):long:this' during 'LSRA allocate' (IL size 4270)

File: /__w/1/s/src/coreclr/jit/lsra.cpp Line: 4050

That's pre-existing - seen in an earlier run (logs link).

Antigen on different targets has quite a few "uncategorized" issues (guessing these are for "wrong result" cases?), but the only x86 failure looks like #64787, and is an inlining assert (that this backend-only change could not have caused, I suppose).

[kunalspathak]

LGTM. Thanks!

[kunalspathak]

@SingleAccretion - Keep an eye on the superpmi-replay run that will kick off on your commit to make sure there are no errors.

[kunalspathak]

Improvements - dotnet/perf-autofiling-issues#3651