[SVFS] Vector Function ABI name demangler.

[fpetrogalli]

Summary

This patch implements the demangling functionality as described in the
Vector Function ABI. This patch will be used to implement the
SearchVectorFunctionSystem(SVFS) as described in the RFC:

http://lists.llvm.org/pipermail/llvm-dev/2019-June/133484.html

A fuzzer is added to test the demangling utility.

[aranisumedh]

Minor Update. Cosmetic changes.

[fpetrogalli]

Hi all,

thank you for the reviews so far.

I am picking up this change-set to continue working on it. Thank you @aranisumedh for your work!

Question: what is the fuzzer supposed to do? Generate random strings to be parsed by VABI::getVectorName?

Francesco

[fpetrogalli]

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h

29 ↗

(On Diff #214419)

@lebedev.ri, @jdoerfert I agree that the names ParameterKind and ParamType are too generic to be exposed in the llvm namespace, but I think that moving them in the VABI namespace might not be the right thing to do. These enum and structure are supposed to support other types of parameters of vector function that might not be OpenMP specific or directly related to a vector function ABI specification. This is one of the goals of the RFC submitted for the SVFS.

Therefore I suggest to move them back to the llvm namespace, with names that will make it clar that we are talking about function vectorization.

How about the following?

ParameterKind -> VectorFunctionParameterKind or VFParameterKind
ParamType -> VectorFunctionParameter or VFParameter (we might want get rid of the Type token too)

Does that make sense to you?

[fpetrogalli]

I have uploaded a new version with the fuzzer requested by @lebedev.ri .

[fpetrogalli]

Cosmetic changes to a comment.

[jdoerfert]

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h

29 ↗

(On Diff #214419)

How about the following?

ParameterKind -> VectorFunctionParameterKind or VFParameterKind
ParamType -> VectorFunctionParameter or VFParameter (we might want get rid of the Type > token too)

Does that make sense to you?

That is fine with me (both the VectorFunction or the VF version). It would be nice and consistent to put VF (or VectorFunction) in front of all these names (ParameterKind, ISAKind, ParamType) as done for VectorFunctionShape.

[lebedev.ri]

Sorry, meant to reply, but lost track.

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h

29 ↗

(On Diff #214419)

I think that would be good-enough.

211 ↗

(On Diff #218144)

files should end with newlines

llvm/tools/vfabi-demangle-fuzzer/vfabi-demangler-fuzzer.cpp

19 ↗

(On Diff #218144)

Question: does getVectorName() run *all* of the parsing code in SearchVectorFunctionSystem.cpp?

23–26 ↗

(On Diff #218144)

Yes, this should be ok i suppose.

[fpetrogalli]

I have updated the code according to the last round of reviews. I have removed pieces of code that were not needed anymore for testing.

The parser now is split from the VFParameter class (the old ParamType class).

[fpetrogalli]

Hi @sdesmalen:

it would be better to parse the mangled string incrementally, rather than extracting each feature from the string individually.

I think the parser now does what you ask for the "variable length" part of the mangled string, which is the one that holds the <vlen> and the <parameters>.

Francesco

llvm/tools/vfabi-demangle-fuzzer/vfabi-demangler-fuzzer.cpp

19 ↗

(On Diff #218144)

Good catch. Now it does.

[fpetrogalli]

I have done the following updates:

1. Added demangling and tests for uniform parameters ("u").
2. Added more docs, with references to the Vector Function ABI specs for x86 and AArch64.
3. Use default constructors for the structs introduced in the patch.
4. Added a test that checks that the demangling functions for the parameters is the same for the vector extensions of both AArch64 and x86.

[lebedev.ri]

Not a review, but some thoughts nonetheless.

llvm/include/llvm/Analysis/VectorUtils.h

64–66 ↗

(On Diff #219420)

have you considered std::tie() ? Not sure if it would work/help here.

71 ↗

(On Diff #219420)

Documentation commends have 3x /

75–82 ↗

(On Diff #219420)

VectorVariant

But, i think this function should return llvm::Optional<Some Struct>, or error_or maybe

104 ↗

(On Diff #219420)

This is unreachable, right? Else i think this might need better error propagation strategy.

164 ↗

(On Diff #219420)

same, and elsewhere.

[fpetrogalli]

llvm/include/llvm/Analysis/VectorUtils.h

75–82 ↗

(On Diff #219420)

I have changed the signature to use a single input parameter. The method is still returning a bool.

I have used optional in the static method that creates the VFShape instance.

104 ↗

(On Diff #219420)

I have replace the output type with Optional<VFParamKind> and moved the llvm_unreachable at the end of the function.

164 ↗

(On Diff #219420)

Done. The static method now returns an optional value.

[fpetrogalli]

In this new patch I have:

1. Fixed a build failure of the fuzzer (I forgot to apply the interface changes of the last diff).
2. Add ASSERT_DEATH testing for invalid names.
3. Add testing to make sure <scalarname> is always present in the string.
4. Add testing to make sure that <vectorname> is present when the opening ( is parsed.
5. Add testing to make sure that the string terminates with ) when <vectorname> is present.

[fpetrogalli]

I have deferred the handling of the mask token "M" to the VFParameter struct, introducing a new VFPAramKind that handles global function predication. This should be more in line with the aim of this implementation, which is to make VFShape independent from the Vector Function ABI. In particular, there are use cases that @simoll cares about where predication can be attached to single parameters, and not to the whole function.

Tests to make sure the Global Predicate is added correct;ly has been added for all the targets supported in this patch (x86 and aarch64).

[jdoerfert]

I'm fine with this (one small nit below).

@simoll, @lebedev.ri, any more comments or can this go in?

llvm/lib/Analysis/VFABIDemangling.cpp

413 ↗

(On Diff #219773)

Why is this an Optional? It seems to always return a proper VFParamKind if it returns at all.

[lebedev.ri]

Looks ok.
(Note that i'm only looking at code, not correctness of demangling here.)

llvm/include/llvm/Analysis/VectorUtils.h

63 ↗

(On Diff #219773)

There's ongoing effort to migrate to llvm::Alignment (i don't recall the name)

94 ↗

(On Diff #219773)

I'm not sure why this should not also return Optional<ParsedData>.

[simoll]

One minor thing: VFABI parsing should succeed for any well-formed AVFBI vector function name (beyond what's listed in the VFISAKind enum). This would open up the functionality for external users.

llvm/include/llvm/Analysis/VectorUtils.h

50 ↗

(On Diff #219773)

How about renaming to ISA_Unknown ?

llvm/lib/Analysis/VFABIDemangling.cpp

302 ↗

(On Diff #219773)

Why not carry on with parsing a well-formed AFABI String if the ABI is unknown?
This would enable support for VFABIs not listed here.

[fpetrogalli]

I have address all comments. Thank you for your feedback @jdoerfert @simoll @lebedev.ri !

Additionally, I have renames the enums values of VFISAKind from VFISAKind::ISA_* to VFISAKind::*.

llvm/lib/Analysis/VFABIDemangling.cpp

413 ↗

(On Diff #219773)

yeah, sorry,Optional is an unnecessary complication here.

[simoll]

LGTM. Thanks!

[sdesmalen]

Thanks for all the changes @fpetrogalli, this patch is really taking shape! Just added a few more drive-by comments.

llvm/include/llvm/Analysis/VectorUtils.h

94 ↗

(On Diff #219773)

nit: Can we rename this main function to something like tryDemangleForVFABI ?

116 ↗

(On Diff #219940)

Given that ScalarName and VectorName are not part of the equality-comparison, does that imply they should not be part of VFShape ?

llvm/lib/Analysis/VFABIDemangling.cpp

30 ↗

(On Diff #219940)

Why is it named getISA which starts parsing at position4 rather than tryParseISA which starts parsing at position 0?

314 ↗

(On Diff #219940)

Can we make the dropping of these substrings part of getMask and getISA (on success) ?

356 ↗

(On Diff #219940)

nit: this comment seems unnecessary (the code says it all)

[fpetrogalli]

llvm/include/llvm/Analysis/VectorUtils.h

116 ↗

(On Diff #219940)

I see. How about I refactor this as follow?

struct VFShape {
  unsigned VF;     // Vectorization factor.
  bool IsScalable; // True if the function is a scalable function.
  VFISAKind ISA;   // Instruction Set Architecture.
  SmallVector<VFParameter, 8> Parameters; // List of parameter informations.
};

struct VFInfo {
  VFShape Shape;
  StringRef ScalarName;                   // Scalar Function Name.
  StringRef VectorName; // Vector Function Name associated to this VFShape.
;

I think this makes more sense, I just want to double check with you before applying the changes.

[fpetrogalli]

HI @sdesmalen ,

I have updated the patch according to your feedback. In particular, I have modified all the parsing method to use the ParseRet enum instead of booleans. It looks better now, thanks.

Once outstanding work is the comparison operator of VFShape. I have proposed a solution in a comment, let me know what you think, I will apply it if you agree on the approach. BTW: good catch. The VFShape should carry information only about the shape, not about the names of the functions.

One more nit: while working on the SVFS, I have noticed that the llvm_unreachable I have added at the end of the static method to create FShape out of the parsed data was quite annoying: it got fired every time the CI I was trying to vectorize didn't have the name with the mangled attribute. hence, I have removed the assertion, and converted the ASSERT_DEATH tests into EXPECT_FALSE tests in TEST(VectorFunctionABITests, OnlyValidNames). This should not be controversial (I am happy to be told I am wrong if that's the case).

Thank you,

Francesco

[sdesmalen]

In D66024#1668515, @fpetrogalli wrote:

I have updated the patch according to your feedback. In particular, I have modified all the parsing method to use the ParseRet enum instead of booleans. It looks better now, thanks.

Agreed, this looks good now, thanks!

Once outstanding work is the comparison operator of VFShape. I have proposed a solution in a comment, let me know what you think, I will apply it if you agree on the approach. BTW: good catch. The VFShape should carry information only about the shape, not about the names of the functions.

Yes, that seems like a good suggestion.

One more nit: while working on the SVFS, I have noticed that the llvm_unreachable I have added at the end of the static method to create FShape out of the parsed data was quite annoying: it got fired every time the CI I was trying to vectorize didn't have the name with the mangled attribute. hence, I have removed the assertion, and converted the ASSERT_DEATH tests into EXPECT_FALSE tests in TEST(VectorFunctionABITests, OnlyValidNames). This should not be controversial (I am happy to be told I am wrong if that's the case).

That seems like the better choice given how you've shaped the interface that returns a Optional<VFShape>, but I don't really understand why this would break CI.

[fpetrogalli]

In D66024#1669011, @sdesmalen wrote:

One more nit: while working on the SVFS, I have noticed that the llvm_unreachable I have added at the end of the static method to create FShape out of the parsed data was quite annoying: it got fired every time the CI I was trying to vectorize didn't have the name with the mangled attribute. hence, I have removed the assertion, and converted the ASSERT_DEATH tests into EXPECT_FALSE tests in TEST(VectorFunctionABITests, OnlyValidNames). This should not be controversial (I am happy to be told I am wrong if that's the case).

That seems like the better choice given how you've shaped the interface that returns a Optional<VFShape>, but I don't really understand why this would break CI.

The way I am building the VFShape is as follows:

const StringRef AttrString =
        CI->getCalledFunction()->getFnAttribute("vector-function-abi-variant").getValueAsString();
      SmallVector<StringRef, 8> ListOfStrings;
      AttrString.split(ListOfStrings, ",");
SmallVector<VFShape, 8> Ret;
      for (auto MangledName : ListOfStrings) {
	auto Shape = VFShape::getFromVFABI(MangledName);
        if (Shape.hasValue() && Shape.getValue().ScalarName == ScalarName)
          Ret.push_back(Shape.getValue());
      }

Most of the functions don't have the attribute, which result in list of string consisting of a single element, which is an empty string, therefore the parser fails and the exception is raised.

Maybe there is a better way to do it, but given the Optional<VFShape> return, it doesn't make sense to raise the exception on failure.

[fpetrogalli]

I have split the implementation of VFShape into VFShape and VFInfo, as agreed.

I have also found a couple of bugs thanks to the fuzzer (that was a great suggestion @lebedev.ri !). I have added the correspondent unit tests to prevent regressions. The bugs were around invalid mangled name strings and the parsing of the align token.

Regards,

Francesco

[fpetrogalli]

I added the Unknown ISA handling in the test ISAIndependentMangling, and updated the comments in the fuzzer.

[sdesmalen]

llvm/include/llvm/Analysis/VectorUtils.h

153 ↗

(On Diff #220128)

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.

[fpetrogalli]

llvm/include/llvm/Analysis/VectorUtils.h

153 ↗

(On Diff #220128)

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.

[sdesmalen]

llvm/include/llvm/Analysis/VectorUtils.h

153 ↗

(On Diff #220128)

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.

[fpetrogalli]

I have removed the ParsedData struct as requested by @sdesmalen , my reasoning for keeping it was wrong.

I have updated the tests and the fuzzer accordingly.

[fpetrogalli]

llvm/include/llvm/Analysis/VectorUtils.h

153 ↗

(On Diff #220128)

I agree with you, thank you.

[sdesmalen]

llvm/include/llvm/Analysis/VectorUtils.h

92 ↗

(On Diff #220511)

Sorry, just spotting this now. Instead of having struct VFInfo; here, can you just move the definition of VFInfo here?

136 ↗

(On Diff #220511)

nit: this code is still commented out.

[fpetrogalli]

Restore the function name to tryParseForVFABI.

[sdesmalen]

Thanks @fpetrogalli, LGTM!