[PATCH] D94746: [AMDGPU] Move kill lowering to WQM pass and add live mask tracking

[Carl Ritson via Phabricator via llvm-commits]

critson marked 8 inline comments as done.
critson added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertSkips.cpp:213-214
   MDT->getBase().insertEdge(&MBB, EarlyExitBlock);
+  if (OldSuccessor)
+    MDT->getBase().deleteEdge(&MBB, OldSuccessor);
 }
----------------
foad wrote:
> Seems like dead code since OldSuccessor is never set to anything useful.
This belongs in D94748.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:15
-/// with shader side effects (stores and atomics). This pass is run on the
-/// scheduled machine IR but before register coalescing, so that machine SSA is
-/// available for analysis. It ensures that WQM is enabled when necessary, but
----------------
foad wrote:
> Are you changing whether or not this pass can assume SSA form?
Yes, since the pass is now run after scheduler drop support for SSA form.
Supporting live mask tracking is much simpler non-SSA (no need to add virtual registers and PHIs tracking through every program block).
While I could support both, the code in WQM pass would be very large for a behaviour we are not using.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:150
   DenseMap<const MachineInstr *, InstrInfo> Instructions;
-  MapVector<MachineBasicBlock *, BlockInfo> Blocks;
-  SmallVector<MachineInstr *, 1> LiveMaskQueries;
+  DenseMap<MachineBasicBlock *, BlockInfo> Blocks;
+
----------------
foad wrote:
> I think this was changed to a MapVector to give a stable iteration order, so changing it back to DenseMap seems dangerous.
It seems that change occurred during the development of this patch.  I missed that, so failed to incorporate it.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:618-626
+bool SIWholeQuadMode::canSplitBlockAt(MachineBasicBlock *BB, MachineInstr *MI) {
+  // Cannot split immediately before the epilog
+  // because there are values in physical registers
+  if (MI->getOpcode() == AMDGPU::SI_RETURN_TO_EPILOG) {
+    return false;
+  }
+
----------------
piotr wrote:
> Unused?
This is legacy of an older version that should have been deleted.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:866
+  LIS->InsertMachineInstrInMaps(*EarlyTermMI);
+  if (WQMMaskMI)
+    LIS->InsertMachineInstrInMaps(*WQMMaskMI);
----------------
piotr wrote:
> WQMMaskMI is always nullptr here.
Yep, this should be in D94747.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:1048
                               MachineBasicBlock::iterator Before,
-                              unsigned SavedOrig) {
+                              Register SavedOrig, char NonWWMState) {
   MachineInstr *MI;
----------------
piotr wrote:
> piotr wrote:
> > It looks the new param NonWWMState is unused?
> I can see you are going to use it in D94747, but as it stands now it will cause a build warning.
Move to D94746.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94746