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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4181-4182
+    if (ConstSrc->isNullValue())
+      // Optimization: ballot(0) returns all zeroes.
+      return DAG.getConstant(0, DL, VT);
+    if (ConstSrc->isOne()) {
----------------
nhaehnle wrote:
> foad wrote:
> > arsenm wrote:
> > > Should also do this in instcombine, as icmp/fcmp already do
> > I've had a go, but is it really necessary to implement the same optimization cases in two places (especially the one that just replaces a ballot intrinsic with a read_register intrinsic), and how do I test them independently?
> Independent tests as opt tests should go into test/Transforms/InstCombine. You can find existing tests for the icmp/fcmp intrinsics there.
> 
> Nitpick: please add braces for the if-body (multiple lines guarded by if).
Yes. If I were to pick one, it would probably be instcombine. I think it’s unlikely this would come up in a context only visible after lowering. Reducing intrinsics is useful when reducing testcases, and any code in the DAG is just going to need to be redone in globalisel eventually 


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4196
+                              DAG.getCondCode(ISD::SETNE));
+  return DAG.getZExtOrTrunc(SetCC, DL, VT);
+}
----------------
foad wrote:
> arsenm wrote:
> > This is already the right type, so this should be unnecessary
> Does anything guarantee that it's the right type? Is it OK for the compiler to crash if someone writes a .ll file that uses it with the wrong type?
Ideally we would verify wave32 IR somewhere and error, but I don’t think that exists today


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65088