[AMDGPU] Run hazard recognizer pass later

[kerbowa]

Summary

If instructions were removed in peephole passes after the hazard recognizer was
run it is possible that new hazards could be introduced.

Fixes: SWDEV-253090

[rampitec]

LGTM

[arsenm]

Needs test

[t-tye]

Is this now running after the waitcnt insertion pass? That would avoid the NOPs currently being inserted to split memory clauses that are not necessary as the waitcnt instructions will split the clauses.

[kerbowa]

In D89077#2320507, @t-tye wrote:

Is this now running after the waitcnt insertion pass? That would avoid the NOPs currently being inserted to split memory clauses that are not necessary as the waitcnt instructions will split the clauses.

It was running after the waitcnt pass before.

[kerbowa]

In D89077#2320507, @t-tye wrote:

Is this now running after the waitcnt insertion pass? That would avoid the NOPs currently being inserted to split memory clauses that are not necessary as the waitcnt instructions will split the clauses.

We also insert nops in the post-RA scheduler.

[t-tye]

In D89077#2320512, @kerbowa wrote:

In D89077#2320507, @t-tye wrote:

Is this now running after the waitcnt insertion pass? That would avoid the NOPs currently being inserted to split memory clauses that are not necessary as the waitcnt instructions will split the clauses.

We also insert nops in the post-RA scheduler.

In earlier conversation it was suggested that the spurious NOPs were explained as happening because the hazard recognizer inserted them to break memory clauses, and then the waitcnt pass ran. There would be no need to insert the NOPs if the waitcnt instructions were already there. So seems that was not a valid explanation, perhaps the post-RA scheduler is an explanation, but I am unclear why it put those ones in. @rampitec can you help explain?

[rampitec]

In D89077#2320691, @t-tye wrote:

In D89077#2320512, @kerbowa wrote:

In D89077#2320507, @t-tye wrote:

Is this now running after the waitcnt insertion pass? That would avoid the NOPs currently being inserted to split memory clauses that are not necessary as the waitcnt instructions will split the clauses.

We also insert nops in the post-RA scheduler.

In earlier conversation it was suggested that the spurious NOPs were explained as happening because the hazard recognizer inserted them to break memory clauses, and then the waitcnt pass ran. There would be no need to insert the NOPs if the waitcnt instructions were already there. So seems that was not a valid explanation, perhaps the post-RA scheduler is an explanation, but I am unclear why it put those ones in. @rampitec can you help explain?

Post-RA scheduler and hazard recognizer is the same pass if you run post-RA scheduler. If not it is a separate pass.

[foad]

In D89077#2320804, @rampitec wrote:

In D89077#2320691, @t-tye wrote:

In D89077#2320512, @kerbowa wrote:

In D89077#2320507, @t-tye wrote:

Is this now running after the waitcnt insertion pass? That would avoid the NOPs currently being inserted to split memory clauses that are not necessary as the waitcnt instructions will split the clauses.

We also insert nops in the post-RA scheduler.

In earlier conversation it was suggested that the spurious NOPs were explained as happening because the hazard recognizer inserted them to break memory clauses, and then the waitcnt pass ran. There would be no need to insert the NOPs if the waitcnt instructions were already there. So seems that was not a valid explanation, perhaps the post-RA scheduler is an explanation, but I am unclear why it put those ones in. @rampitec can you help explain?

Post-RA scheduler and hazard recognizer is the same pass if you run post-RA scheduler. If not it is a separate pass.

Just to expand on this, GCNHazardRecognizer is not a pass, it's a class that gets plugged into two different passes:

1. The post-RA scheduler (which runs before waitcnt insertion)
2. The standalone hazard recognizer pass (which should be run pretty late, after any other pass that could introduce hazards)

The post-RA scheduler can insert NOPs which turn out to be unnecessary because a waitcnt will be inserted there anyway, which would fix the hazard. I reckon the right way to fix this is to teach GCNHazardRecognizer to return Hazard (an "advisory" hazard) instead of NoopHazard (a "mandatory" hazard) when it is called from the post-RA scheduler. That way, the post-RA scheduler gets to avoid some hazards by reordering instructions (but not inserting nops), and the rest of the hazards get fixed in the late hazard recognizer pass by inserting nops. I have tried to implement this in the past but never quite finished it.

[foad]

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

1037

I am nervous about running PostRAHazardRecognizer after SIInsertHardClauses because you get undefined behaviour if you insert random instructions of the wrong type into the middle of a hard clause. In particular it is illegal to include s_waitcnt inside a clause, and I know the hazard recognizer does insert s_waitcnt_depctr in some cases.

SIInsertHardClauses does bundle the clauses it forms, but the hazard recognizer can still insert instructions inside bundles in some cases. (I think it is wrong to do this, and I have some ideas about fixing it.)

So I'm not sure if this will cause a problem in practice but it makes me nervous. It seems like we have more than one pass that wants to run as late as possible and I'm not sure how to resolve that. I know @nhaehnle has asked about this in the past.

[foad]

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

1037

I've run some Vulkan CTS testing and this patch doesn't seem to cause any problems in practice, so I won't block it.

[kerbowa]

Add test.

[t-tye]

Will there be a separate review to parametrize the hazard recognizer so that when run early it only resolves the hazards necessary for the register allocator et al. That allows the other hazards to only be resolved in the final run. This would avoid splitting memory clauses early before the waitcnt/invalidate instructions have been inserted that may themselves split the memory clauses.

llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

 }
 
 void GCNPassConfig::addPreEmitPass() {
   addPass(createSIMemoryLegalizerPass());
   addPass(createSIInsertWaitcntsPass());
   addPass(createSIShrinkInstructionsPass());
   addPass(createSIModeRegisterPass());
 
+  if (getOptLevel() > CodeGenOpt::None)
+    addPass(&SIInsertHardClausesID);
+
+  addPass(&SIRemoveShortExecBranchesID);
+  addPass(&SIInsertSkipsPassID);
+  addPass(&SIPreEmitPeepholeID);
   // The hazard recognizer that runs as part of the post-ra scheduler does not
   // guarantee to be able handle all hazards correctly. This is because if there
   // are multiple scheduling regions in a basic block, the regions are scheduled
   // bottom up, so when we begin to schedule a region we don't know what
   // instructions were emitted directly before it.
   //
   // Here we add a stand-alone hazard recognizer pass which can handle all
   // cases.
   //
   // FIXME: This stand-alone pass will emit indiv. S_NOP 0, as needed. It would
   // be better for it to emit S_NOP <N> when possible.
   addPass(&PostRAHazardRecognizerID);
-  if (getOptLevel() > CodeGenOpt::None)
-    addPass(&SIInsertHardClausesID);
-
-  addPass(&SIRemoveShortExecBranchesID);
-  addPass(&SIInsertSkipsPassID);
-  addPass(&SIPreEmitPeepholeID);
   addPass(&BranchRelaxationPassID);
 }
 
 TargetPassConfig *GCNTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new GCNPassConfig(*this, PM);
 }
 
 yaml::MachineFunctionInfo *GCNTargetMachine::createDefaultFuncInfoYAML() const {

llvm/test/CodeGen/AMDGPU/hazard-pass-ordering.mir

+# RUN: llc -march=amdgcn -mcpu=gfx908 -start-before=si-pre-emit-peephole %s -o - | FileCheck -check-prefix=GCN %s
+
+# Verify that the dedicated hazard recognizer pass is run after late peephole
+# optimizations. New hazards can be introduced if instructions are removed by
+# passes that are run before the final hazard recognizer.
+
+---
+# GCN-LABEL: {{^}}mai_hazard_pass_ordering_optimize_vcc_branch:
+# GCN: v_accvgpr_read_b32
+# GCN-NEXT: s_nop
+# GCN-NEXT: flat_load_dword
+name:            mai_hazard_pass_ordering_optimize_vcc_branch
+body:             |
+  bb.0:
+    $vgpr0 = V_MOV_B32_e32 1, implicit $exec
+    $vgpr2 = V_ACCVGPR_READ_B32 killed $agpr0, implicit $exec
+    $sgpr8_sgpr9 = S_MOV_B64 -1
+    $vgpr3 = FLAT_LOAD_DWORD $vgpr0_vgpr1, 0, 0, 0, 0, implicit $exec, implicit $flat_scr
+    $vcc = S_ANDN2_B64 $exec, killed renamable $sgpr8_sgpr9, implicit-def dead $scc
+    S_CBRANCH_VCCNZ %bb.1, implicit killed $vcc
+
+  bb.1:
+    S_ENDPGM 0
+...