[PATCH] D73286: [llvm][VectorUtils] Tweak VFShape for scalable vector functions.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM, please address the nits mentioned here https://reviews.llvm.org/D73286#1845416



================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:170
+Optional<VFInfo> tryDemangleForVFABI(StringRef MangledName,
+                                     const Module *M = nullptr);
 
----------------
fpetrogalli wrote:
> jdoerfert wrote:
> > fpetrogalli wrote:
> > > jdoerfert wrote:
> > > > Any reason one would not pass `M` here? If not, I would remove the default and maybe even make it required (via reference).
> > > Could do - that will require changing all unit tests to provide a module though... are you happy for me to do that?
> > I guess we can script it* so it should not be too bad work-wise. The question is, what do we want to achieve. If there is a use case that we want to support going forward which does not need the module, I'm fine with the pointer. If we are actually not anticipating such a use case we will just burden and confuse our future selves as we open up the possibility for all kinds of subtle bugs as someone forgot somewhere to pass a module, we should at least remove the default value.
> > 
> > * Something like
> > ```
> > for file in $(ag --files-with-matches "tryDemangleForVFABI")
> > do
> >   sed -i -e 's:tryDemangleForVFABI(\([^)]*\)):tryDemangleForVFABI(\1, nullptr):' ${file}
> > done
> > 
> > ```
> > (UNTESTED)
> On the long term I have a plan to require the Module, because I think to make sense to store the `FunctionType` of the variant instead of the pair `VF` and `Scalable` in `VFShape`. This is to support cases in which the vectorization factor is not uniform across the signature of the vector function - for example, where one parameter has two lanes and the other one four. Admittedly, such cases are not supported by the current vectorizer and cannot be represented via the VFABI rules, but we want to make `VFShape` generic enough so that it does not prevent adding such support if needed.
> 
>  I'd rather not do this change now, because I think this will go beyond the scope of this patch, where all what I am trying to do is to make sure that `VF` is never zero when demangling the vector function.
> 
> So I have opted for passing `const Module &` to the demagler, and for adding an assertion right after having discovered the vector name to be used that makes sure that the name is present in the module itself. I'll finish up this soon and upload it.
OK. Let's keep it for now this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73286