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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:208
   void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<SlotIndexes>();
     AU.addRequired<LiveIntervals>();
----------------
I don't think you need to "require" this since you don't explicitly use it for anything.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:638
+  case AMDGPU::S_MOV_B64:
+    TermMI->setDesc(TII->get(AMDGPU::S_MOV_B64_term));
+    break;
----------------
Maybe just choose the new opcode inside the switch, and pull the get/setDesc calls out?


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:685
+  case ISD::SETGT:
+    Opcode = AMDGPU::V_CMP_GT_F32_e64;
+    break;
----------------
I'm a bit sceptical of some of the opcodes in this table.

E.g. SETOGT --(swap operands)--> SETOLT --(invert)-->SETUGE which is V_CMP_NLT, but the table here says V_CMP_GT.

It might be more obvious what's going on if you call getSetCCSwappedOperands and getSetCCInverse first, and then have a simple lookup from SETxx to opcode. Or you could avoid the "invert" by using AND instead of ANDN2?


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:700
+  case ISD::SETONE:
+  case ISD::SETNE:
+    Opcode = AMDGPU::V_CMP_EQ_F32_e64;
----------------
It's a bit more conventional to lump SETNE together with SET**U**NE (unlike all the other comparisons which go with the **O** form), though of course it doesn't really matter.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:1052
   // anything.
-  if (!isEntry && BI.Needs == StateWQM && BI.OutNeeds != StateExact)
+  if (!isEntry && BI.Needs == StateWQM && BI.OutNeeds != StateExact) {
     return;
----------------
No need to add braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94746