[PATCH] D78203: [VP,Integer,#2] ExpandVectorPredication pass
Simon Moll via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 4 01:35:01 PDT 2020
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
More information about the llvm-commits
mailing list