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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:1079
+  if (Size == 16 && !ST.has16BitInsts())
     return -1;
+
----------------
The test won't pass on, say, hawaii because of lack of 16 bit insts. It'll fail to select.
I guess it's a separate issue where we should probably legalize the intrinsic (= widen its input to i32) for those archs?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp:1084
+    if (Size == 16)
+      return ST.hasTrue16BitInsts() ? TrueS16Opc : S16Opc;
+    if (Size == 32)
----------------
Note: not sure if "true" or "false" 16 bit insts matter.
I did both to be safe but I'd be curious to know if it's actually needed.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstructions.td:893
+//  out in the test.
+//   Skipped pattern: Dst MI def isn't a register class(COPY:{ *:[i64] } ?:{ *:[i1] }:$src)
 def : Pat <
----------------
Help needed here, not sure how to get this one to work. I tried a lot of things, including COPY_TO_REGCLASS and nothing seems to do the trick.
Due to the simplicity of the pattern I'm wondering if it isn't better to just do it manually in the InstructionSelector rather than fight the GISel TableGen emitter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136448