[PATCH] D130677: [AMDGPU] Fix DGEMM hazard for GFX90a

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp:2316
     const int MaxWaitStates = 19;
+    const int DMFMABetweenVALUWriteVMEMRead = 2;
 
----------------
Move it one line above, MaxWaitStates is always last.


================
Comment at: llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp:2344
+      if (IsMem && ST.hasGFX90AInsts() && !ST.hasGFX940Insts()) {
+        if (TRI.isVectorRegister(MF.getRegInfo(), Use.getReg())) {
+          auto IsDGEMMHazard = [this](const MachineInstr &MI) {
----------------
There is already `Reg` variable available here.


================
Comment at: llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp:2344
+      if (IsMem && ST.hasGFX90AInsts() && !ST.hasGFX940Insts()) {
+        if (TRI.isVectorRegister(MF.getRegInfo(), Use.getReg())) {
+          auto IsDGEMMHazard = [this](const MachineInstr &MI) {
----------------
rampitec wrote:
> There is already `Reg` variable available here.
Hoist MRI initialization out of the loop.


================
Comment at: llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp:2353
+            // starts with DGEMM.
+            if (NextMI == MI.getParent()->end()) {
+              const MachineBasicBlock* CurrMBB = MI.getParent()->getSingleSuccessor();
----------------
This is an overkill and way insufficient at the same time (what if an instruction is a meta/debug?).

`getWaitStatesSince` will traverse instructions backwards one by one and take care about all of that. It will either see DGEMM before the offending VALU or not. In fact since it is only 2 waitstates DGEMM must be the first instruction it will see, otherwise you can immediately bail. To bail immediately you could use custom IsExpired function, or just capture a variable to detect a DGEMM. Whatever you prefer.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSubtarget.h:194
   bool HasImageGather4D16Bug = false;
+  bool HasDGEMMVALUWriteMemOpBug = false;
   bool HasVOPDInsts = false;
----------------
Unused.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D130677/new/

https://reviews.llvm.org/D130677