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

[David Sherwood via Phabricator via llvm-commits]

david-arm 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)
----------------
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.


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

https://reviews.llvm.org/D136251