[PATCH] D66024: [SVFS] Vector Function ABI name demangler.

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked 3 inline comments as done.
fpetrogalli added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VectorUtils.h:153
+      const VFABI::ParsedData PD = OptPD.getValue();
+      const VFShape Shape({PD.VF, PD.IsScalable, PD.ISA, PD.Parameters});
+      return VFInfo({Shape, PD.ScalarName, PD.VectorName});
----------------
sdesmalen wrote:
> fpetrogalli wrote:
> > sdesmalen wrote:
> > > Can't tryDemangleForVFABI return a `Optional<VFInfo>` directly? (rather than construct it here from the `ParsedData`)
> > > This also makes me question the need for `ParsedData`.
> > I could, but I think it is not the right thing to do. I expect the the`VFInfo` struct to grow beyond handling Vector Function ABI  and OpenMP information (it is one of the requirements). When this happens, all we need to do is just to to change this line adding a proper ParsedData -> VFInfo constructor instead of having to adapt the parser to the new VFInfo class.
> If `VFInfo` turns into a class that supports a superset of the VFABI, the parser could construct a `VFInfo` object that populates less fields through having multiple constructors for `VFInfo`, or through a constructor with default arguments for the fields that are not applicable.
> In the end there needs to be code somewhere that fills in the VFInfo object. Having a layer in the middle that just passes the information around offers little value, unless that layer adds some new functionality. Without such functionality, I think it makes more sense to return `VFInfo` directly.
I agree with you, thank you.


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

https://reviews.llvm.org/D66024