[PATCH] D78203: [VP,Integer,#2] ExpandVectorPredication pass

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/include/llvm/IR/PredicatedInst.h:34
+
+class PredicatedInstruction : public User {
+public:
----------------
Can this inherit from Instruction? Its a little surprising that it has Instruction in its name but has to be casted to Instruction.


================
Comment at: llvm/include/llvm/IR/PredicatedInst.h:50
+
+  void *operator new(size_t s) = delete;
+
----------------
Can we move this above copyIRFlags to keep all the deleted things together and away from code that does stuff.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:45
+/// \returns Whether the vector mask \p MaskVal has all lane bits set.
+static bool IsAllTrueMask(Value *MaskVal) {
+  auto ConstVec = dyn_cast<ConstantVector>(MaskVal);
----------------
Should this start with lower case 'i'?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:70
+  // smallest integer type.
+  if (TTI.getRegisterBitWidth(true) == 0)
+    return MinLaneBits;
----------------
Add /* Vector */ comment to the true.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:76
+  unsigned MaxLaneBits = std::min<unsigned>(
+      IntegerType::MAX_INT_BITS, TTI.getRegisterBitWidth(true) / StaticVL);
+
----------------
Save the register bit width instead of querying it again?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:98
+  assert(DivTy->isIntOrIntVectorTy());
+  return Constant::getAllOnesValue(DivTy);
+}
----------------
This isn't safe for sdiv/srem is it? INT_MIN/-1 is overflow and will trap.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:247
+      Builder.CreateBinOp(static_cast<Instruction::BinaryOps>(PI.getOpcode()),
+                          FirstOp, SndOp, PI.getName(), nullptr);
+
----------------
Do we need the nullptr here? I had to lookup what argument that is.


================
Comment at: llvm/lib/IR/PredicatedInst.cpp:28
+bool PredicatedInstruction::canIgnoreVectorLengthParam() const {
+  auto VPI = dyn_cast<VPIntrinsic>(this);
+  if (!VPI)
----------------
I think we prefer "auto *" for pointers?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78203