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

[Simon Moll via Phabricator via llvm-commits]

simoll planned changes to this revision.
simoll marked 12 inline comments as done.
simoll added a comment.

@craig.topper Thanks for your comments! I'll update shortly.



================
Comment at: llvm/include/llvm/IR/PredicatedInst.h:34
+
+class PredicatedInstruction : public User {
+public:
----------------
craig.topper wrote:
> Can this inherit from Instruction? Its a little surprising that it has Instruction in its name but has to be casted to Instruction.
Conceptually, `PredicatedInstruction` is a super class of `Instruction`: An `Instruction` is a `PredicatedInstruction` with the mask/evl parameters hard-wired to "all lanes true". That's why we can't make Instruction an ancestor of this one.
I'll clarify this with a comment.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:70
+  // smallest integer type.
+  if (TTI.getRegisterBitWidth(true) == 0)
+    return MinLaneBits;
----------------
craig.topper wrote:
> Add /* Vector */ comment to the true.
This really should have its own enum.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:76
+  unsigned MaxLaneBits = std::min<unsigned>(
+      IntegerType::MAX_INT_BITS, TTI.getRegisterBitWidth(true) / StaticVL);
+
----------------
craig.topper wrote:
> Save the register bit width instead of querying it again?
Moved this function in to the CachingVPExpander class and added a lane bits cache there.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78203