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

[Graham Hunter via Phabricator via llvm-commits]

huntergr 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
----------------
rkruppe wrote:
> 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?
Ah, I spotted the same bug you did (missing implementation for bool), but it seems I hadn't submitted the comment I wrote. I tried it with nesting a pair of unsigned ints and that worked, but making it work directly with ElementCount seems a nicer idea, thanks.


https://reviews.llvm.org/D32530