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

[Robin Kruppe via Phabricator via llvm-commits]

rkruppe added a comment.

Thank you! I took another look and found two nits, sorry for not pointing them out earlier.

Other than those nits I'd say "LGTM", but note that I currently have neither commit access nor even any accepted upstream patches, so a LGTM from me probably isn't enough to commit.



================
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 };
----------------
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.


================
Comment at: lib/IR/LLVMContextImpl.h:1311
   DenseMap<std::pair<Type *, uint64_t>, ArrayType*> ArrayTypes;
-  DenseMap<std::pair<Type *, unsigned>, VectorType*> VectorTypes;
+  DenseMap<std::pair<Type *, uint64_t>, VectorType*> VectorTypes;
   DenseMap<Type*, PointerType*> PointerTypes;  // Pointers in AddrSpace = 0
----------------
Nit: have you considered `std::pair<bool, unsigned>` instead of manually bit-packing it into 64 bits? DenseMap should support nested pairs, the size should be the same (except if unsigned is 64 bit, which I don't believe we support and which is extremely niche anyway), and it would simplify `VectorType::get` a little bit.


https://reviews.llvm.org/D32530