Tested on 10k pipelines from Vulkan games, this patch reduces the number of v_writelane instructions from 9842 to 6440 (at the expense of using more loads of course).

Then, as you say, our register allocation needs to be intelligent enough to keep the matrices packed.
How would you define the instructions for this to work?

Corrected the use of isCopyInstr.

I have done the first round of testing of the patch on graphics content, with LLPC PatchBufferOp disabled and small modification in LLPC to use ptr addrspace (7) in lieu of lgc.buffer.desc.to.ptr.

Thanks for the extra info; I understand the problem now: currently there seems to be no way to take advantage of the opsel bit to reuse the same destination matrix registers for two wmma instructions.

Delete only if all copies are dead, use isCopyInstr.

Thanks, that would avoid the regression. However, I still do not fully understand the failing mode - can you show the test case + extra code that triggers the issue?

Ping.

Thanks for working on this. Just a few high-level comments:

Reworked to check for copy.

Ping - I think the only unresolved point is potential weakening of the generic check.

Rebased.

I am trying to understand the failing case better. Can the issue only happen with the extra patch with packing? Is the issue only with zeroinitializers (constant matrices), or it is just where the problem was found?

I am open to suggestions how to handle this more gracefully.

The test case is dependent on AMDGPU-specific D154083, which rematerializes instructions with wide registers. I was not able to observe the erroneous behaviour without it, but should be possible to trigger it somehow.

Pre-committing the test for bug fix in D156999. The test is dependent on AMDGPU-specific D154083, which rematerializes instructions with wide registers.

Thanks, AMD changes LGTM. I spotted this weirdness recently when staring at some remat code for AMDGPU. (It's not a serious issue for us, because the generic check is really conservative for our target, and we typically do not want to block the remat when the generic check returns true).

In D154083#4516643, @arsenm wrote:

In D154083#4493643, @piotr wrote:

In D154083#4490691, @arsenm wrote:

Could really use a MIR test that shows this. Also would be nice to have some evil cases, where the result register is tied to the input pointer register

This patch is now based on a test update (https://reviews.llvm.org/D154816), where I am also adding a new test that exercises the shrinking - test_remat_s_load_dword_immx16_subreg.

Can you describe the evil case(s) in more detail? Do you mean S_LOAD_DWORDX16_IMM with tied-def, or something else?

Yes. I don't think subregisters with tied operands are particularly well defined, but I was thinking something like %0:sreg_256 = S_LOAD_DWORDX16 %0.sub0_sub1

Added new case with the same reg used for use and def: %0.sub0_sub1:sgpr_256 = S_LOAD_DWORDX2_IMM %0.sub0_sub1:sgpr_256.

Added more tests.

Ran extensive testing on graphics workloads, which uncovered some bugs. Added fixes and more tests for those interesting cases in D154816.

In D154083#4490691, @arsenm wrote:

Could really use a MIR test that shows this. Also would be nice to have some evil cases, where the result register is tied to the input pointer register

Note: I am aware the "DEFAULT" prefix is currently unused, but was not sure if the intention was for it to be used in the near future - so I am leaving it there for now.

• Addressed review comments.
• Relaxed check to include all invariant loads, not only dereferenceable ones.
• Rebased patch over the commit with new/changed tests.
• Updated MMO with new size in the shrinking path.

Having looked at some real-world graphics content on AMDGPU (with ScalarizeMinBits = 32), I can confirm the usefulness of this patch. I can see more packed instructions generated (for example v_pk_add_f16, v_pk_mul_f16, v_pk_fma_f16).

Yes - this is NFC, but paves the way for other changes.

LGTM with a nit, but please wait for Matt's approval.

In D151703#4380561, @arsenm wrote:

What we should do is teach rematerialization to split scalar loads

Thanks for working on this. Just added a couple of nits.

LGTM - looks cleaner than the version before the change.

In D137066#4198531, @tsymalla wrote:

What is the current state of this?

Would be good to add a test case, but I do realize this can be tricky.

We're seeing some regression coming from this change for AMDGPU backend.

(fixing the spelling error)

Will do extra testing.

Updating patch to address two more issues detected by running tests in debug mode:

• wrong opcodes used in getVGPRSpillSaveOpcode in the original patch
• tests using insufficient spill slot size in my patch

Thanks - the extra set of tests has uncovered another issue in code.

Ping.