[PATCH] D69891: [VP,Integer,#1] Vector-predicated integer intrinsics

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a subscriber: sstefan1.
jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:1172
+
+}
+
----------------
simoll wrote:
> jdoerfert wrote:
> > I guess you can add readnone as an attribute, also nosync and nofree please.
> The intrinsics are `readnone` already. `nosync` seems sensible.
I asked @sstefan1 to add nosync and nofree to the td file and other places needed to use them. That should make your changes more concise.

---

I would have argued we could even do the `#inline` trick here, or maybe some .td magic, to avoid this replication, but I will not force anything.


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:1
+//===-- llvm/Instruction.def - File that describes Instructions -*- C++ -*-===//
+//
----------------
Nit: copy&past


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:27
+#define HANDLE_VP_TO_OC(VPID,OC)
+#endif
+
----------------
To be honest, I would have made the Opcode for vp intrinsics of existing functions `VP_EXISTING` (or similar) and used a single macro definition here, assuming all vp intrinsics so far have a unique corresponding scalar version.
These are two unrelated points you can consider (I won't force either).

(If you device to do both changes, you can even get away by only defining the existing opcode in the intrinsic "macro definition").


================
Comment at: llvm/include/llvm/IR/VPIntrinsics.def:89
+#undef REGISTER_VP_INTRINSIC
+#undef HANDLE_VP_TO_OC
----------------
Nit: I'd run clang format on these files as well. Should minimize diffs in the long run.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:244
+  return GetStaticVectorLengthOfType(VPMask->getType());
+}
+
----------------
Does it make sense to have this corner case in here or should we just make it an `Optional<int64_t>` and call it a day?

You can also remove the lambda if you move the code, except there is a reason to keep it.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:263
+  default:
+    return None;
+
----------------
Default clauses make it harder to find all switches that might need to be adjusted. If there is no strong reason to have them we should not.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:293
+    //  [..]
+    //   return true;
+#define REGISTER_VP_INTRINSIC(VPID, MASKPOS, VLENPOS) \
----------------
Leftover?


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:306
+  default:
+    return Instruction::Call;
+
----------------
I doubt this is a sensible default. I doubt one should return anything for a non-vp intrinsic.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:337
+    return true;
+  }
+
----------------
Nit: braces.


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:353
+}
+
 Instruction::BinaryOps BinaryOpIntrinsic::getBinaryOp() const {
----------------
Nit: clang format your changes (or patches) (clang-format.py for vim has the formatdiff option)


================
Comment at: llvm/unittests/IR/VPIntrinsicTest.cpp:20
+
+namespace llvm {
+namespace {
----------------
Not: `using namsepace llvm;` is probably less confusing here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69891