[PATCH] D88081: [AMDGPU] Move WQM Pass after MI Scheduler

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:315-332
+      // Does this def cover whole register?
+      const MachineOperand &DstOp = MI->getOperand(0);
+      bool DefinesFullReg = false;
+      if (DstOp.isReg()) {
+        DefinesFullReg = (DstOp.getReg() == Reg) &&
+                         (!DstOp.getSubReg() || DstOp.getSubReg() == SubReg ||
+                          DstOp.isUndef());
----------------
I don't understand the logic here. Why the special treatment of operand 0?


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:358-372
+    // It is possible that a tied use may exist for a register entirely
+    // (re)defined by this instruction if it was added by SI IMG Init pass. Such
+    // a use should not be followed otherwise this instruction will be marked
+    // unnecessarily.
+    if (Use.isImplicit() && Use.isTied()) {
+      Register SubReg = Use.getSubReg();
+      bool IsDef = false;
----------------
I don't understand this logic. A use is a use -- why should implicitness or tiedness make a difference? This seems pretty wrong.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:868-884
+      // Running after control flow lowering we must fix up control flow
+      // to consider modified EXEC mask.
+      if (PrevMI && MI.isTerminator() && BI.OutNeeds == StateExact) {
+        if (MI.getOpcode() == XorTermrOpc &&
+            MI.getOperand(0).getReg() == Exec &&
+            PrevMI->getOpcode() == OrSaveExecOpc) {
+          assert(!MRI->isSSA());
----------------
This looks suspicious. Can you please explain what is happening here?


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:1018
+          LIS->removeInterval(Reg);
+          DefMI->getOperand(0).setReg(Reg);
+          MI->removeFromParent();
----------------
This is incorrect, there could be other users of the value. Just keep the simpler case below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88081