[PATCH] D32530: [SVE][IR] Scalable Vector IR Type

[Robin Kruppe via Phabricator via llvm-commits]

rkruppe added inline comments.


================
Comment at: include/llvm/IR/DerivedTypes.h:498
+    uint64_t MinimumEltCnt = getNumElements();
+    assert(MinimumEltCnt <= UINT_MAX && "Too many elements in vector");
+    return { (unsigned)MinimumEltCnt, Scalable };
----------------
huntergr wrote:
> rkruppe wrote:
> > Nit: This restriction on the range of NumElements is very reasonable, but we should make it a proper invariant of the type and enforce it in `VectorType::get` rather than post-hoc in some accessors.
> VectorType::get already has this restriction, at least on most platforms -- the argument to it is 'unsigned', which is usually 32 bits. The ElementCount struct also uses 'unsigned'. It may be worth changing it to an explicit uint32_t. 
> 
> I think VectorType originally stored the number of elements as a 32 bit field so was consistent with the interface, but at some point the different SequentialType variants were changed to unify with a single 64 bit size field in the parent class.
> 
> I will create an updated patch to check that we don't overflow when using methods like getDoubleElementsVectorType; good catch.
Oh, right, I was under the mistaken impression that VectorType::get took uint64_t. So the potential overflow in getDoubeElementsVectorType is pre-existing, but if you want to take the time to harden against it now, that's great.

I don't think changing unsigned to uint32_t is worth the churn, at least not in this patch.


https://reviews.llvm.org/D32530