[PATCH] D65088: [AMDGPU][RFC] New llvm.amdgcn.ballot intrinsic

Fri Mar 20 10:51:03 PDT 2020

arsenm added a comment.

In D65088#1933161 <https://reviews.llvm.org/D65088#1933161>, @Flakebi wrote:

> I removed the COPY_TO_REGCLASS, it looked flaky and does not work with GlobalISel.
>  Instead, the ballot intrinsic is morphed into an AMDGPUISD::SETCC. Compares are the only reasonable way to get a boolean value into the wavefront form as an i32/i64 and use it in LLVM.
>
> Matt: How do I add these combines for GlobalISel?
>  Should they go into AMDGPUCombine.td? And do I have to write functions for this or does it work purely with pattern matching?

If you're not going to have a fallback pattern in tablegen, then the GlobalISel support is probably a separate patch. This would not go in AMDGPUCombines.td as this is not an optimization combine. By having everything go through AMDGPUISD::SETCC, this develops the same problem the existing icmp/fcmp intrinsics have for GlobalISel support. As a separate patch, we need to replace how AMDGPUISD::SETCC works to avoid relying on the CondCode fiel

================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:8693-8695
+SDValue
+SITargetLowering::performIntrinsicWOChainCombine(SDNode *N,
+                                                 DAGCombinerInfo &DCI) const {
----------------
Since you're now treating this as the full lowering path, and not an optimization for the special case with a compare, this should go in LowerINTRINSIC_WO_CHAIN

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:3901-3902
+        // amdgcn.ballot(i1 1) is exec.
+        Function *NewF = Intrinsic::getDeclaration(
+            II->getModule(), Intrinsic::read_register, II->getType());
+        const char *RegName = "exec";
----------------
Should move the getDeclaration below to where you actually use it. If you happened to hit the other size break case, you would have introduced a dead declaration

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65088