[PATCH] D75672: [ValueTypes] Add support for scalable EVTs
Cullen Rhodes via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 17 05:43:31 PDT 2020
c-rhodes added inline comments.
================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:265
unsigned getVectorNumElements() const {
- assert(isVector() && "Invalid vector type!");
+ assert((isVector() || isScalableVector()) && "Invalid vector type!");
if (isSimple())
----------------
efriedma wrote:
> c-rhodes wrote:
> > efriedma wrote:
> > > c-rhodes wrote:
> > > > efriedma wrote:
> > > > > Not sure changing getVectorNumElements like this makes sense.
> > > > I've removed this change, there was just one place in SelectionDAGBuilder I had to fix up to call `getVectorElementCount`. I had a chat with Graham and he suggested it would be good to check if it's a fixed-length vector here, maybe by having something similar to the MVT `isFixedLengthVector` on the EVT which could call `MVT::isFixedLengthVector` if it's a `SimplyTy` or check it's not a scalable vector otherwise.
> > > Yes, we want to catch incorrect use of getNumElements in both IR and in SelectionDAG.
> > Is this something we can implement going forward or should this be handled before landing this patch?
> If it's easy, might as well handle it here, if there's some non-obvious complication, I'd be fine with putting it off.
I've tested the following patch downstream:
```
diff --git a/llvm/include/llvm/CodeGen/ValueTypes.h b/llvm/include/llvm/CodeGen/ValueTypes.h
index 72e0cc8..c2db003 100644
--- a/llvm/include/llvm/CodeGen/ValueTypes.h
+++ b/llvm/include/llvm/CodeGen/ValueTypes.h
@@ -152,6 +152,10 @@ namespace llvm {
return isSimple() ? V.isScalableVector() : isExtendedScalableVector();
}
+ bool isFixedLengthVector() const {
+ return isSimple() ? V.isFixedLengthVector() : isExtendedFixedLengthVector();
+ }
+
/// Return true if this is a 16-bit vector type.
bool is16BitVector() const {
return isSimple() ? V.is16BitVector() : isExtended16BitVector();
@@ -262,7 +266,7 @@ namespace llvm {
/// Given a vector type, return the number of elements it contains.
unsigned getVectorNumElements() const {
- assert(isVector() && "Invalid vector type!");
+ assert(isFixedLengthVector() && "Invalid vector type!");
if (isSimple())
return V.getVectorNumElements();
return getExtendedVectorNumElements();
@@ -432,6 +436,7 @@ namespace llvm {
bool isExtended1024BitVector() const LLVM_READONLY;
bool isExtended2048BitVector() const LLVM_READONLY;
bool isExtendedScalableVector() const LLVM_READONLY;
+ bool isExtendedFixedLengthVector() const LLVM_READONLY;
EVT getExtendedVectorElementType() const;
unsigned getExtendedVectorNumElements() const LLVM_READONLY;
ElementCount getExtendedVectorElementCount() const LLVM_READONLY;
diff --git a/llvm/lib/CodeGen/ValueTypes.cpp b/llvm/lib/CodeGen/ValueTypes.cpp
index dcb3a88..a2a35e2 100644
--- a/llvm/lib/CodeGen/ValueTypes.cpp
+++ b/llvm/lib/CodeGen/ValueTypes.cpp
@@ -106,6 +106,10 @@ bool EVT::isExtendedScalableVector() const {
return isExtendedVector() && cast<VectorType>(LLVMTy)->isScalable();
}
+bool EVT::isExtendedFixedLengthVector() const {
+ return isExtendedVector() && !cast<VectorType>(LLVMTy)->isScalable();
+}
+
EVT EVT::getExtendedVectorElementType() const {
assert(isExtended() && "Type is not extended!");
return EVT::getEVT(cast<VectorType>(LLVMTy)->getElementType());
```
After running check-all there are a lot of failures. Ideally we want to get rid of `getVectorNumElements` but a quick grep shows there's >450 uses of it in lib/CodeGen and lib/Target/AArch64 alone. All of these probably aren't `EVT::getVectorNumElements` but having looked through some of the errors downstream there's cases like those in `SelectionDAGBuilder::visitShuffleVector` that need fixing up.
I think this assert is similar to the implicit cast of `TypeSize` -> `uint64_t` which Sander addressed with a warning in D75297. Being restrictive here is also going to trigger a lot of asserts, perhaps we can also emit a warning here?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75672/new/
https://reviews.llvm.org/D75672
More information about the llvm-commits
mailing list