[JIT] Optimize constant V512 vector with broadcast

[Ruihan-Yin]

This PR is trying to solve #90328.

The optimization is implemented by replacing the constant V512 vector by a V128 and a broadcasti128 node when lowering GT_STOREIND plus an eligible constant V512 vector as its operand.

Currently, the implementation only covers V512 -> broadcasti128(V128), we are open to adjust the implementation or bring more situations into this PR, ideally V512/256 -> broadcasti128(V128), when AVX512 is available. (Possibly plus V512 -> broadcast64x4(V256).)

[ghost]

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

Issue Details

---

This PR is trying to solve #90328.

The optimization is implemented by replacing the constant V512 vector by a V128 and a broadcasti128 node when lowering GT_STOREIND plus an eligible constant V512 vector as its operand.

Currently, the implementation only covers V512 -> broadcasti128(V128), we are open to adjust the implementation or bring more situations into this PR, ideally V512/256 -> broadcasti128(V128), when AVX512 is available. (Possibly plus V512 -> broadcast64x4(V256).)

Author:	Ruihan-Yin
Assignees:	-
Labels:	area-CodeGen-coreclr
Milestone:	-

[Ruihan-Yin]

Ran the test suite twice, should be some known or random fails, turning PR to ready for review.

[Ruihan-Yin]

Hi @EgorBo, this PR is ready for review, please see if this PR is able to cover #90328, thanks!

[tannergooding]

Is this going to perform better or just save on the size of the rodata section?

What about on hardware without AVX-512 (Haswell, Skylake, etc)?

What about scalar->V128, scalar->V256, scalar->V512, V128->V256, V256->V512, etc?

For AVX-512, the scalar->Vector scenario can at least be covered by an embedded broadcast. But for other scenarios, this seems like its trading more instructions for smaller data section.

[Ruihan-Yin]

Is this going to perform better or just save on the size of the rodata section?

The expected improvement is saving some memory space for constant values.

What about on hardware without AVX-512 (Haswell, Skylake, etc)?

I was intended to use VBROADCASTI32X4, which is an AVX512 only instruction, but seems VBROADCASTI128 can also do this job for V256->V128.

What about scalar->V128, scalar->V256, scalar->V512, V128->V256, V256->V512, etc?

I presume the scope is to achieve compressing larger existing constant vector to smaller vector in a pure memory operation, which embedded broadcast might not be able to handle.

If we want to take compressing to scalar into consideration, we might also have the opportunity: V128/256/512 ->Byte/Word/DWord/QWord.

For AVX-512, the scalar->Vector scenario can at least be covered by an embedded broadcast. But for other scenarios, this seems like its trading more instructions for smaller data section.

From my understanding of #90328, the issue is for a pure store instruction case, then the code gen is mostly:

vmovups zmm, zmmword ptr[constant section]
vmovups zmmword ptr[target], zmm

the optimization is essentially replacing the first load with a broadcast instruction with a smaller constant operand.

I might get the issue wrong or incompletely, so please correct me if I have any misunderstanding.

[tannergooding]

the optimization is essentially replacing the first load with a broadcast instruction with a smaller constant operand.

👍, if its primarily for the case where we'd otherwise have a vmovups reg1, [addr] then it sounds great to replace that with vbroadcast reg1, [addr] where possible.

I was initially concerned it would also change:

vadd reg1, reg2, [addr]

into

vbroadcast reg3, [addr]
vadd reg1, reg2, reg3

[Ruihan-Yin]

the optimization is essentially replacing the first load with a broadcast instruction with a smaller constant operand.

👍, if its primarily for the case where we'd otherwise have a vmovups reg1, [addr] then it sounds great to replace that with vbroadcast reg1, [addr] where possible.

I was initially concerned it would also change:

vadd reg1, reg2, [addr]

into

vbroadcast reg3, [addr]
vadd reg1, reg2, reg3

I think it wouldn't cover that case (at least it is not intended to cover), as the entry point of this opt is LowerStoreIndir().

[Ruihan-Yin]

Fail should be unrelated.

Hi, @tannergooding @EgorBo, I added the optimization for V512->V256 and V256->V128, and I think it reaches the expected coverage and ready for the reviews.

[JulieLeeMSFT]

@tannergooding, this community PR is ready to review. PTAL.

[tannergooding]

I believe you can just do:

Suggested change

	if (!node->Data()->AsVecCon()->TypeIs(TYP_SIMD32) && !node->Data()->AsVecCon()->TypeIs(TYP_SIMD64))
	if (!node->Data()->AsVecCon()->TypeIs(TYP_SIMD32, TYP_SIMD64))

[tannergooding]

LGTM. This should get a secondary review from someone on the JIT team

CC. @dotnet/jit-contrib

[tannergooding]

CC. @jakobbotsch, @EgorBo in particular

[Ruihan-Yin]

Hi @jakobbotsch @EgorBo, this PR is ready for review, would you please take a look? Thanks!

[jakobbotsch]

Since this is AVX-512 backend work I think @BruceForstall should take a look... On my quick glance it seemed a bit odd to do it during STORE_INDIR lowering when presumably constants can benefit in many other cases (as long as they're not already contained), but I am not very familiar with these instructions. Also going to close and reopen this to rerun CI.

[BruceForstall]

Diffs

[Ruihan-Yin]

Thanks everyone for reviewing on this!