[PATCH] D136448: [AMDGPU][GISel] Add llvm.amdgcn.icmp selection

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

LGTM except for trying to handle i1 intrinsics



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:1237
+  //  in i1 handling between GISel and DAG.
+  if (Size == 1)
+    Size = 32;
----------------
Pierre-vh wrote:
> arsenm wrote:
> > Pierre-vh wrote:
> > > Pierre-vh wrote:
> > > > This is obviously wrong as we only want to compare the first bit in each VGPR, but I don't know what instruction to use here yet.
> > > > 
> > > > Which instruction do I need to use here? V_CNDMASK ?
> > > > 
> > > CNDMASK doesn't work because it constrains the operands to 64 bits, but the i1 are widened to 32.
> > > We need something that would do `dst[threadI] = v0.i1`, if I understand correctly?
> > Where are you seeing i1 VGPRs? Those should just not happen. At one point I might have been trying to handle that, but I think it's basically wrong for it to see the selector 
> I don't see i1 VGPRs, but `vgpr(s1)` as the arguments of the iccmp
That shouldn't happen. The intrinsic shouldn't accept incoming i1 values. If you try to use i1 with these intrinsics, it should just fail selection 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136448