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

[Carl Ritson via Phabricator via llvm-commits]

critson 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());
----------------
nhaehnle wrote:
> I don't understand the logic here. Why the special treatment of operand 0?
Are you saying I should iterate MI->defs() instead?
The code here is intended to mark all instructions defining parts of the specified register.
If this is a partial register write then we need to follow the input values to mark the other instructions.


================
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;
----------------
nhaehnle wrote:
> I don't understand this logic. A use is a use -- why should implicitness or tiedness make a difference? This seems pretty wrong.
Agreed, this should go away.
It was an early hack before I wrote a working markDefs.


================
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());
----------------
nhaehnle wrote:
> This looks suspicious. Can you please explain what is happening here?
This is analogous to the code above that modifies the SI_ELSE to make it respect the EXEC mask -- it is a very special case match and fix up based on current code generation so I do not like it either.
If we make SI_ELSE always respect modifications to the EXEC mask then this can go away.  Then perhaps we add a late peephole to clear up some of the unnecessary instructions when they are not required.
Do you have an opinion on this?


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

I am not going to fight to keep this, but we would benefit from more late clean up of unnecessary copies.
I guess this ties into some of the things I am touching on in D89187, so the follow up to that might solve this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88081