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

[Robin Kruppe via Phabricator via llvm-commits]

rkruppe added inline comments.


================
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
----------------
huntergr wrote:
> rkruppe wrote:
> > 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.
> So I tried this, and couldn't compile it -- there's no implementation of getHashValue, getEmptyKey, getTombstoneKey, etc. for the nested pair.
> 
> In our downstream compiler this is actually implemented as a dense map of a 3-element tuple against the Type*, for which we have implemented the appropriate extensions to DenseMapInfo.
> 
> If that approach is preferred to bitpacking, I'll make a separate patch to implement the DenseMap extensions.
I finally got a chance to look into the error you're seeing and it turns out the root cause is not nested pairs but a missing implementation of DenseMapInfo for bool.

We could add that implementation, but in the future other code may also want to hash an ElementCount, e.g. VPlan may migrate the vectorization factor VF from `unsigned` to ElementCount and there are some DenseMaps with VF as key. With this in mind I'm leaning towards implementing `DenseMapInfo<VectorType::ElementCount>`, using the bit fiddling that's currently open-coded here. What do you think?


https://reviews.llvm.org/D32530