[PATCH] D81925: [AMDGPU] Select s_cselect

[Piotr Sobczak via Phabricator via llvm-commits]

piotr marked 6 inline comments as done.
piotr 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);
----------------
arsenm wrote:
> This should not be changed. We now report zero extended booleans since a few months ago
Thanks, I was confused by this, mainly because this global change here did not cause any issues in my tests. 

I rewrote this part and moved the relevant functionality to lowerSelect.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:5564-5566
+  for (MachineInstr &CandI :
+       make_range(std::next(MachineBasicBlock::reverse_iterator(Inst)),
+                  Inst.getParent()->rend())) {
----------------
arsenm wrote:
> 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
Added the check.

Yes, this patch is not as neat as I would like due to the extra handling of SCC.


================
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(); }]
----------------
arsenm wrote:
> 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.
Sounds good. 

Btw, some s_cselect's are already generated for the global-isel at this point. It was just the old isel that did not have any support for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81925