Repository navigation
Support emitting a constant for the Vector64/128/256.Create hardware intrinsic methods #10033
Description
For the case where the Vector64.Create, Vector128.Create, and Vector256.Create helper functions are called with all constant arguments, we should support emitting a constant which can be loaded from memory, rather than emitting a chain of shuffle or insert calls.
We may also find some benefit in doing the same for partial constants as a partial constant with several inserts can still be faster than treating it as non-constant.
category:cq
theme:vector-codegen
skill-level:intermediate
cost:medium
Reacted by Jacek Blaszczynski, Frank Niemeyer, damageboy and Ganbarukamo41
Activity
tannergooding commentedon Mar 26, 2018
FYI. @CarolEidt, @fiigii
fiigii commentedon Mar 26, 2018
We may need more scenarios and profiling data to support this statement...
tannergooding commentedon Mar 26, 2018
It may be worth noting that MSVC, GCC, and Clang all do this.
They do it for both
_mm_set1and_mm_set. However, I imagine it is only beneficial for the latter (_mm_set), since the former (_mm_set1) is generally a single broadcast or shuffle.I do agree we should do the appropriate profiling/etc, but the native compilers doing it provide a good precedent.
4creators commentedon Mar 26, 2018
It's beneficial for both and is done by all native compilers. Shuffle is cheap but broadcast mostly not.
CarolEidt commentedon Mar 27, 2018
There are many things that native compilers do that the JIT doesn't. We need to justify this kind of work with actual scenarios.
tannergooding commentedon Mar 27, 2018
@CarolEidt, this is basically the same as: https://github.com/dotnet/coreclr/issues/17256 (just replace
= new Vector4(1, 2, 3, 4)with= SetVector128(1, 2, 3, 4))I can come up with a bunch of scenarios showing the bad codegen here and actual numbers if it's needed.
fiigii commentedon Mar 28, 2018
The literals of
float/doublehave to be read from memory, but integers can be encoded in the instructions. So,SetVector128/256should have different codegen heuristics fromVector4.tannergooding commentedon Mar 28, 2018
Right, so for
float/double, it should be more efficient to do a single read, when all four values are constant.Could you elaborate?
None of integer SIMD load instructions that I can find (
movd,movq,movdqa,movdqu, etc) support immediates. So at best, you will be emitting amov reg, imm64,pinsrq xmm, reg, immper 8-bytes (on new hardware, with more complex code involving shifting on older hardware).fiigii commentedon Mar 28, 2018
Yes, I meant the codegen of "evaluating the integer constants" rather than the SIMD instruction 😄
Yes, we have to store each constant vector in a memory slot. If a program has many distinct constant vectors, it would spend much memory. So, there is tread-off...
mikedn commentedon Mar 28, 2018
I doubt there can be so many distinct constant vectors in a program that it would matter. The real problem is that each method is getting its own data section and thus its own constants. And even inside the same data section constants are not deduplicated so that the same constant is emitted multiple times. Also add the lack of proper packing into the mix and things start to look a bit risky.
All this already happens today for float/double constants but at least those are much smaller...
tannergooding commentedon Mar 28, 2018
I'm not so sure on this part. It looks like it generally takes more bytes to do the insert/shift code than it does to store the raw bytes and read from memory.
So, even with "perfect" deduping of float constants (which we don't have), we still only have a code savings of ~2-bytes.
mikedn commentedon Mar 28, 2018
Hmm, right,
SetVectortends to generate so much code that either way things aren't that great in terms of size. The concerns about data size would be more relevant toSetAll, that one generates much less code and yet native compilers tends to also use memory constants for that as well. But then native compilers have the luxury of deduplicating constants...[-]Support emitting a constant for certain SetVector hardware intrinsic helpers[/-][+]Support emitting a constant for the Vector64/128/256.Create hardware intrinsic methods[/+]tannergooding commentedon Nov 21, 2019
I've modified this to track
Vector*.Createas theSetVector*methods no longer exist and were changed to the above instead.CarolEidt commentedon Nov 21, 2019
It looks like dotnet/coreclr#27909 gives us a scenario to motivate this.
mikedn commentedon Nov 22, 2019
Note that what I did with
Vector4& co. may be insufficient/too limited. The proper solution might require addingGT_CNS_SIMD.damageboy commentedon Dec 1, 2019
I also have a bunch of constants that could use this.
I managed to these into hand-written ReadOnlySpan proeperty getters + LoadDquVector256().
While the use of
ReadOnlySpan<byte>initially looks risky, given that these intrinsics are x86 only anyway, and the constant are obviously x86 only as well, it's an ugliness I'm willing to bear.This mostly alleviates the pain of Vector256.Create() with constants, which is quite considerable.
tannergooding commentedon Apr 5, 2020
I'd like to see if I can get this done as it should help codegen in a few places.
Would we want to try and mimic dotnet/coreclr#14393, which basically just handles this in lowering by replacing the node with a
GT_CLS_VAR_ADDRand an emitted constant or would we want to introduce a newGT_CNS_*node type (GT_CNS_SIMD, as suggested above, orGT_CNS_VECmight be good names) and handle this earlier (maybe in importation or morph)?CC. @CarolEidt, @dotnet/jit-contrib
tannergooding commentedon May 8, 2020
Resolved with #35857