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

[Sam Kolton via Phabricator via llvm-commits]

SamWot marked an inline comment as done.
SamWot added inline comments.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:276
+        continue;
+      } else if (PotentialMI != PotentialMO.getParent()) {
+        return nullptr;
----------------
rampitec wrote:
> You only need to do:
> 
> ```
> if (PotentialMI != PotentialMO.getParent()) {
>   return nullptr;
> ```
> If != nullptr and continue are not needed.
Rewrote potentialToConvert function so that they should be more clear.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:384
+  for (MachineInstr &MI : MBB) {
+    unsigned Opcode = MI.getOpcode();
+    switch (Opcode) {
----------------
rampitec wrote:
> What will happen if instruction already comes to the pass with either SDWA or DPP in it? There can be only one such operand per instruction.
Currently instruction with SDWA or DPP would not be converted. They would not pass "isConvertibleToSDWA" check.
Later it would be nice to extend this pass so that it would accept SDWA instructions.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:579
+  MachineInstrBuilder SDWAInst =
+    BuildMI(*MI.getParent(), MI, MI.getDebugLoc(),SDWADesc);
+
----------------
rampitec wrote:
> rampitec wrote:
> > Space after comma.
> You are working in pre-regalloc... I think you need to update LIS when you create or delete instructions.
First LiveIntervalAnalysis is runnig after my pass. Liveness informatioin is inavailable for my pass.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:646
+  for (auto &Operand : SDWAOperands) {
+    Operand->convertToSDWA(*SDWAInst, TII);
+  }
----------------
rampitec wrote:
> There can be only one SDWA operand, right? It seems this loop may try to create more than one...
No, there might be several SDWA operands for one instructions. One for each instruction operands.


https://reviews.llvm.org/D30038