[PATCH] D30038: [ADMGPU] SDWA peephole optimization pass.

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:276
+    MachineInstr *PotentialMI = MRI->getVRegDef(Source->getReg());
+    assert(PotentialMI);
+    // Check if this instructions are in same basic block
----------------
I'm not sure it is always a vreg. It is better got giveup on physreg. The same for multiply defined vregs, you are running the pass late and can have multiple defs. Just bail out.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:335
+      auto SDWASrc = std::make_shared<SDWASrcOperand>(Src1, Dst, WORD_1);
+      dbgs() << "Match: " << MI << "To: " << *SDWASrc;
+      SDWAOperands[&MI] = std::move(SDWASrc);
----------------
Should be wrapped in DEBUG().


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:364
+
+    default: continue;
+    }
----------------
Not needed.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:466
+  if (!ST.hasSDWA() ||
+      !AMDGPU::isVI(ST)) { // FIXME: We don't support SDWA anywhere other than VI
+    return false;
----------------
arsenm wrote:
> This check is unnecessary?
```
>= VOLCANIC_ISLANDS?
```


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:477
+  // FIXME: For now we only combine instructions in one basic block
+  for (MachineBasicBlock &MBB: MF) {
+    SDWAOperands.clear();
----------------
Space before colon. Here and in other places.


https://reviews.llvm.org/D30038