[PATCH] D136251: [LoopVectorize] Use available masked vector functions when required

[Graham Hunter via Phabricator via llvm-commits]

huntergr added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:285
+    // otherwise we look for one that matches the supplied VF.
+    auto Mappings = VFDatabase::getMappings(CI);
+    for (VFInfo Info : Mappings)
----------------
david-arm wrote:
> huntergr wrote:
> > david-arm wrote:
> > > I wonder if it's useful to have a `VFDatabase::getMaskedMappings` variant or something like that? That way we can avoid walking through all the mappings if there aren't going to be any masked variants anyway. I was thinking about something like this:
> > > 
> > >   auto Mappings = VFDatabase::getMaskedMappings(CI);
> > >   if (Mappings.emtpy())
> > >     return false;
> > >   for (VFInfo Info : Mappings)
> > >     if (!VF || Info.Shape.VF == *VF)
> > >       return true;
> > >   return false;
> > > 
> > > Do you think it's worth it?
> > getMaskedMappings would just end up iterating over all the variants anyway -- this code doesn't really do any caching, since it's a static method rather than an instance method. So every time we ask for variants we re-decode the mangled names to figure out which functions we should be looking at.
> > 
> > I think LoopVectorize and VectorUtils could be much smarter about this, but that's something for another patch.
> Sure, I was just thinking we could avoid walking over the mappings twice like we're doing at the moment. With your patch as it stands we call `VFDatabase::getMappings`, which walks over all the mappings and creates a set of them, which we then iterate over yet again here. However, if you had a function such as `VFDatabase::getMaskedMappings` you can at least optimise for the (common) case where there are no masked variants, i.e.
> 
> ```auto Mappings = VFDatabase::getMaskedMappings(CI);
> if (Mappings.emtpy())
>   return false;```
> 
> That only requires walking over the mappings once, and if there are masked mappings it also makes the second list walk shorter because you've already filtered out the non-masked variants.
> 
> Anyway, I wouldn't hold the patch up for this - I just thought it seemed like it might reduce computational effort.
I agree it would be nice, which is why I'm planning another patch to refactor the use of VFDatabase in LV. My basic idea is to establish an initial mapping between callinsts and vector variants during legality checking so that we only look up valid variants for callinsts in LV itself.

I'd then like to create a mapping for CallInst+VF to Cost+WideningDecision, similar to what's done for memory operations now. This should greatly reduce the amount of redundant lookups -- it also looks through VFDatabase each time it requests a cost for any VF.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D136251/new/

https://reviews.llvm.org/D136251