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

[Jay Foad via Phabricator via llvm-commits]

foad marked 2 inline comments as done.
foad 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()) {
----------------
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?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:4196
+                              DAG.getCondCode(ISD::SETNE));
+  return DAG.getZExtOrTrunc(SetCC, DL, VT);
+}
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65088