Don't struct-promote opaque vectors

[CarolEidt]

The hardware vector types: Vector64<T>, Vector128<T> and Vector256<T> are declared
as having one or more fields of ulong. However, the JIT shouldn't be promoting these
fields to local variables. It is almost never the right type, and the intrinsics in any
case are not designed to cooperate with promoted fields (i.e. an index of a Vector<ulong>
won't map to the promoted lclVar).

Most importantly, it causes all copies of the vector to be done as 64-bit integer loads
and stores.

Finally, it will be important, as we support vector ABIs, to distinguish the handling of
the fixed-size vectors (Vector2, Vector3 and Vector4) which are considered to be
normal structs of N floats, from the opaque types which will be passed in vector registers.

[mikedn]

Wasn't lvUsedInSIMDIntrinsic supposed to prevent promotion already?

[CarolEidt]

Wasn't lvUsedInSIMDIntrinsic supposed to prevent promotion already?

Yes, but it is not used for the HW intrinsic vectors, and even for the other SIMD types, it is not set if the vector is only copied or passed/returned.

[mikedn]

Yes, but it is not used for the HW intrinsic vectors

Hmm, are the SetOpLclRelatedToSIMDIntrinsic calls in gtNewSimdHWIntrinsicNode unnecessary then?

[CarolEidt]

Hmm, are the SetOpLclRelatedToSIMDIntrinsic calls in gtNewSimdHWIntrinsicNode unnecessary then?

Ah, I missed that. Yes, I think they will now be unnecessary, though one could consider that an optimization of sorts, since it will bypass the second check in the struct promotion test.

[tannergooding]

since it will bypass the second check in the struct promotion test

Might it be worthwhile to enforce we set the flag and assert(!isOpaqueSIMDLclVar(varDsc) on the other code path?

[CarolEidt]

Might it be worthwhile to enforce we set the flag and assert(!isOpaqueSIMDLclVar(varDsc) on the other code path?

I'm not sure what flag or "other code path" you're referring to?

[tannergooding]

I meant keeping if (varDsc->lvIsSIMDType() && varDsc->lvIsUsedInSIMDIntrinsic()) (which requires we set IsUsedInSIMDIntrinsic()) and assert isOpaqueSIMDLclVar(varDesc) for the else case.

But, that wouldn't work for the cases where someone uses Vector128<T> and isn't also using intrinsics. There are also a number of other blocks that would make adding the assert more troublesome; so you can just ignore me 😄

[AaronRobinsonMSFT]

@dotnet-bot test Windows_NT x86 min_opt ryujit CoreCLR Perf Tests Correctness please
@dotnet-bot test Windows_NT x86 Release Innerloop Build and Test please

[CarolEidt]

@dotnet/jit-contrib PTAL

[BruceForstall]

Does this fix a GitHub issue?

[CarolEidt]

Does this fix a GitHub issue?

No, I don't believe so. I discovered this when I was seeing unexpected (poor) codegen while working on #21304.

I should probably check some of the struct-related issues to see if any of them are impacted by this. I have had some system issues so haven't been able to run full diffs.

On linux, I get 113 bytes (-0.002%) reduction in SPC.dll. There are 3 methods (actually 3 instantiations of the same method) that regressed:

• System.Runtime.Intrinsics.Vector128`1[Double][System.Double]:get_Zero() - also the Int32 and Int64 versions
  • This generates a xorps; vmovupd instead of xor;mov;mov since the fields aren't promoted. This is a wash size-wise but then we also get a vzeroupper in the prolog because of the 32-byte operation.

Clearly there's room for improved heuristics such that we could treat these "as if" they are promoted to individual fields of the base type if merited. But the current approach is quite pessimizing.

[CarolEidt]

OK, I finally had a chance to run diffs on Windows (full frameworks and tests this time), and spot-check the results:

• X64 windows: -19855 (-0.02% of base), 2157 total methods with size differences (2131 improved, 26 regressed), 463246 unchanged.
• X86 windows: -20530 (-0.02% of base), 2226 total methods with size differences (2193 improved, 33 regressed), 461433 unchanged

Overall, the reductions are places where we eliminated struct copies of arguments, and replaced pairs of 16-byte moves from memory with 32-byte moves from memory or register.

The regressions are variations on:

• Code size is a mostly wash, except for the addition of one or more vzerouppers.
• CSE effects where we create additional CSEs (e.g. due to framesize being smaller without an extra SIMD lclVar for copying) causing additional register resolution or spill.