[AMDGPU] detect WaW hazards when moving/merging load/store instructions

[hakzsam]

Summary

In order to combine memory operations efficiently, the load/store
optimizer might move some instructions around. It's usually safe
to move instructions down past the merged instruction because the
pass checks if memory operations can be re-ordered.

Though, the current logic doesn't handle Write-after-Write hazards.

This fixes a reflection issue with Monster Hunter World and DXVK.

v2: - rebased on top of master
    - clean up the test case
    - handle WaW hazards correctly

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=40130

[nhaehnle]

That's an interesting catch. Basically a Write-after-Write hazard.

Though this seems like the wrong place to add this check, since the function is specifically called canMoveInstsAcrossMemOp.

I rather suspect that instead addToListsIfDependent needs to be fixed instead, so that when the scan hits the S_CMP_EQ_U32 it recognizes the WAW hazard and adds it to the instructions to be moved.

I also wonder if this is related to D60459.

test/CodeGen/AMDGPU/merge-load-store.mir

62

You should remove most of the LLVM IR from the test file. That should be possible as long as you remove the references to IR from the MIR below, e.g. the :: (dereferenceable invariant load 4 from %ir.arg.kernarg.offset.cast, addrspace 4) and change bb.0.bb to bb.0 etc.

145–146

I don't think this is needed.

173

It should be possible to further reduce this test case, e.g. most of bb.2 seems to be unnecessary for the test.

[ronlieb]

This sure does look like the same problem to me. https://reviews.llvm.org/D60459

[ronlieb]

have you run all the lit tests on this patch ?

[hakzsam]

In D61313#1489386, @ronlieb wrote:

This sure does look like the same problem to me. https://reviews.llvm.org/D60459

D60459 doesn't fix the issue.

[hakzsam]

In D61313#1490515, @ronlieb wrote:

have you run all the lit tests on this patch ?

How do I launch that?
I tested with 'make check' btw, no regressions found!

[arsenm]

In D61313#1491569, @hakzsam wrote:

In D61313#1490515, @ronlieb wrote:

have you run all the lit tests on this patch ?

How do I launch that?
I tested with 'make check' btw, no regressions found!

Those are the same thing

[hakzsam]

In D61313#1489140, @nhaehnle wrote:

That's an interesting catch. Basically a Write-after-Write hazard.

Though this seems like the wrong place to add this check, since the function is specifically called canMoveInstsAcrossMemOp.

I rather suspect that instead addToListsIfDependent needs to be fixed instead, so that when the scan hits the S_CMP_EQ_U32 it recognizes the WAW hazard and adds it to the instructions to be moved.

I also wonder if this is related to D60459.

How do I recognize the WAW when scanning S_CMP_EQ_U32? I'm not sure to follow you.

[nhaehnle]

In D61313#1493137, @hakzsam wrote:

In D61313#1489140, @nhaehnle wrote:

That's an interesting catch. Basically a Write-after-Write hazard.

Though this seems like the wrong place to add this check, since the function is specifically called canMoveInstsAcrossMemOp.

I rather suspect that instead addToListsIfDependent needs to be fixed instead, so that when the scan hits the S_CMP_EQ_U32 it recognizes the WAW hazard and adds it to the instructions to be moved.

I also wonder if this is related to D60459.

How do I recognize the WAW when scanning S_CMP_EQ_U32? I'm not sure to follow you.

The S_AND_B64 gets picked up as an InstToMove because it uses a value from a memory operation.

When scanning further and considering the S_CMP_EQ_U32, the addToListsIfDependent checks if any of the *uses* of S_CMP_EQ_U32 refer to any of the defs of the S_AND_B64. At this point, you also have to check the *defs* of the S_CMP_EQ_U32 against the defs of the S_AND_B64. At that point, the code should be able to recognize that both instructions write to the physical register SCC, and add the S_CMP_EQ_U32 to the instructions to be moved.

[ronlieb]

In D61313#1491568, @hakzsam wrote:

In D61313#1489386, @ronlieb wrote:

This sure does look like the same problem to me. https://reviews.llvm.org/D60459

D60459 doesn't fix the issue.

I tried your latest patch on my test case test/CodeGen/AMDGPU/scc-add-lshl-addc.ll in D60459, and it takes care of my issue.
I certainly prefer your patch to mine :-)

[nhaehnle]

Thanks! This looks good, except I still believe the test case can be simplified.

test/CodeGen/AMDGPU/merge-load-store.mir

62

What about this comment?

[hakzsam]

test/CodeGen/AMDGPU/merge-load-store.mir

62

I think I tried and failed to reduce the test case for some reasons. I will try again.

[hakzsam]

test/CodeGen/AMDGPU/merge-load-store.mir

62

What do you want to remove exactly? The test case seems small.

[nhaehnle]

test/CodeGen/AMDGPU/merge-load-store.mir

62

I think it should be possible to remove all of the LLVM IR. Maybe everything except for a ret void. You'll have to remove the from %ir.tmp1 part in the MIR.

[hakzsam]

v3: - remove most of the LLVM IR

[nhaehnle]

Thanks!

[hakzsam]

Thank you too!
Can you eventually push the patch for me? I think I no longer have commit access since the LLVM relicense stuff.

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

     // registers are in SSA form.
     if (Use.isReg() &&
         ((Use.readsReg() && RegDefs.count(Use.getReg())) ||
+         (Use.isDef() && RegDefs.count(Use.getReg())) ||
          (Use.isDef() && TargetRegisterInfo::isPhysicalRegister(Use.getReg()) &&
           PhysRegUses.count(Use.getReg())))) {
       Insts.push_back(&MI);

test/CodeGen/AMDGPU/merge-load-store.mir

   attributes #0 = { convergent nounwind }
   attributes #1 = { convergent nounwind readnone }
 
+  define amdgpu_kernel void @move_waw_hazards() #0 {
+    ret void
+  }
+
+  attributes #0 = { convergent nounwind }
 ...
 ---
 name:            mem_dependency

     S_SETPC_B64_return undef $sgpr30_sgpr31, implicit %6, implicit %7
 
 ...
+---
+# Make sure Write-after-Write hazards are correctly detected and the
+# instructions moved accordingly.
+# operations.
+# CHECK-LABEL: name: move_waw_hazards
+# CHECK: S_AND_B64
+# CHECK: S_CMP_EQ_U32
+name:            move_waw_hazards
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $sgpr0_sgpr1
+
+    %3:sgpr_64 = COPY $sgpr0_sgpr1
+    %6:sreg_32_xm0_xexec = S_MOV_B32 0
+    %7:sreg_32_xm0 = S_MOV_B32 0
+    %8:sreg_64_xexec = REG_SEQUENCE killed %6, %subreg.sub0, %7, %subreg.sub1
+    %9:sreg_128 = S_LOAD_DWORDX4_IMM killed %8, 0, 0, 0 :: (invariant load 16, addrspace 6)
+    %31:sreg_64_xexec = S_BUFFER_LOAD_DWORDX2_IMM %9, 0, 0, 0 :: (dereferenceable invariant load 4)
+    %10:sreg_32_xm0_xexec = COPY %31.sub0
+    %11:sreg_32_xm0_xexec = COPY killed %31.sub1
+    %12:sreg_32_xm0_xexec = S_BUFFER_LOAD_DWORD_IMM %9, 2, 0, 0 :: (dereferenceable invariant load 4)
+    %13:sreg_64 = V_CMP_NE_U32_e64 killed %11, 0, implicit $exec
+    %15:sreg_64 = V_CMP_NE_U32_e64 killed %12, 0, implicit $exec
+    %17:sreg_64_xexec = S_AND_B64 killed %13, killed %15, implicit-def dead $scc
+    S_CMP_EQ_U32 killed %10, 0, implicit-def $scc
+    %18:sreg_32_xm0_xexec = S_BUFFER_LOAD_DWORD_IMM %9, 3, 0, 0 :: (dereferenceable invariant load 4)
+    S_ENDPGM 0
+...