[PATCH] D81925: [AMDGPU] Select s_cselect

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:609
       BuildMI(MBB, MI, DL, get(AMDGPU::S_CSELECT_B32), DestReg)
-          .addImm(1)
+          .addImm(-1)
           .addImm(0);
----------------
This should not be changed. We now report zero extended booleans since a few months ago


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:642
+      BuildMI(MBB, MI, DL, get(AMDGPU::S_CSELECT_B64), DestReg)
+          .addImm(-1)
+          .addImm(0);
----------------
Should be 1


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:5564-5566
+  for (MachineInstr &CandI :
+       make_range(std::next(MachineBasicBlock::reverse_iterator(Inst)),
+                  Inst.getParent()->rend())) {
----------------
I'm not super comfortable with a search for the def like this, but there probably isn't a better option. I also care less about this code now that GlobalISel does this correctly.

Can you check if SCCSource is undef? I think that's the case this might crash on since there will be no def to find. The other cases should be caught by the verifier


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:6210-6211
+      } else {
+        if (MI.getOpcode() == AMDGPU::S_CSELECT_B32 ||
+            MI.getOpcode() == AMDGPU::S_CSELECT_B64) {
+          // This is an implicit use of SCC. We cannot preserve the edge to
----------------
Probably need to note this is really the expected SCC users to handle, since it's more restricted than checking if there's an scc use


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:6229
+
+  if (SCCUsedImplicitly)
+    BuildMI(*SCCDefInst.getParent(), std::next(SCCDefInst.getIterator()),
----------------
Braces


================
Comment at: llvm/lib/Target/AMDGPU/SOPInstructions.td:470
+  (ops node:$src1, node:$src2),
+  (select SCC, $src1, $src2),
+  [{ return N->getOperand(0)->hasOneUse() && !N->isDivergent(); }]
----------------
I have a patch for GlobalISel to move select matching into patterns, which will be incompatible with this since the type of SCC is switched to i32. I guess this doesn't matter, since we're manually selecting it there already.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81925