[PATCH] D85794: [WIP][llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI]
Vineet Kumar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 13 06:08:03 PDT 2020
vkmr added a comment.
Thanks @fpetrogalli for this proposal! I agree with @rogfer01 and @rengolin in that this approach makes a lot of sense.
I have added a few comments inline.
================
Comment at: llvm/include/llvm/Support/TypeSize.h:85
+ /// One or more elements.
+ bool isVector() const { return Scalable || Min > 1; }
+ ///@}
----------------
Unless there is something explicitly preventing from constructing a `{0, true}` `ElementCount`, this could potentially hide bugs by returning `true` for this case. Constraining `ElementCount` constructor is probably a better way.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2725
Type *Ty = TC->getType();
- Constant *Step = ConstantInt::get(Ty, VF * UF);
+ // This is where we can me the step a runtime constant.
+ Constant *Step = ConstantInt::get(Ty, VF.Min * UF);
----------------
Typo (me -> make)?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6195
+ ElementCount VF) {
+ assert(!VF.Scalable);
// If we know that this instruction will remain uniform, check the cost of
----------------
Missing message for `assert`.
I believe this assert falls under point 4 (When building the potential VFs in VPlan. Making the VPlan generic) in your description of where it would be a functional change to support scalable vectors, right?
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:6225
- if (VF == 1)
+ assert(!VF.Scalable);
+ if (VF.isScalar())
----------------
Missing message in `assert`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D85794/new/
https://reviews.llvm.org/D85794
More information about the llvm-commits
mailing list