Page MenuHomePhabricator

SVFS implementation according to RFC: Interface user provided vector functions with the vectorizer.
Changes PlannedPublic

Authored by aranisumedh on Jul 2 2019, 12:43 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2019, 12:43 PM

Some high level coding comments.

Documentation needed in various places.

Why std::vector and not llvm::SmallVector for example?

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
36

I'd like some "class comment" and also comments on the members. It is not clear to everyone (incl. me) what distinguishes for example LinearValPos and LinearUValPos and why we need that.

49

automatically private.

55

I don't think this is a good idea.

63

) : ParamPos(ParamPos), LinearStepOrPos(0)

Why take a string just to convert it? Shouldn't we expose the conversion as a static member and expect a enum here. That way users can use the conversion if they need to but if they know the enums they want there is no need to go through the string first.

113

One could generate them with a macro if one wanted to:

#declare LOOKUP_FN(NAME) \
   bool is ##NAME () { \
            return ParamKind == ParameterKind::OMP_ ## NAME; \
   }

LOOKUP_FN(Uniform)
...
#undef LOOKUP_FN(NAME)
138

The default case is missing. llvm_unreachable I assume.

151

Make it a struct or do not expose all members.

154

) : M(M) {}

158

No need for the typedef. Again, please add comments to the class and members.

164

(Honest question) Can't we use something like = default or is that a C++ feature we are not allowed to use yet?

llvm/lib/Analysis/SearchVectorFunctionSystem.cpp
21

MangledName.startswith("_ZGV") ?

37

You could add an "ISA_UNKNOWN" case as a default here and assert on that one. Would probably make it slightly easier to maintain. But this is fine as well.

aranisumedh marked an inline comment as done.

Added code to expose the functionality as an Analysis Pass. Integrated changes as suggested in prior code review.

aranisumedh added inline comments.Jul 12 2019, 12:15 PM
llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
55

I forgot to remove the comment. This is incorrect comment.

This constructor is not used for testing.

63

I preferred to use a string as demangling does String parsing. Hence, I thought it would be easier to call a class constructor with the StringRef as it's argument.

138

I don't think StringSwitch Default Case handles llvm_unreachable

164

It is a C++ 20 feature. Hence, I think I cannot use it for now. https://en.cppreference.com/w/cpp/keyword/default

Second round, we make progress :)

Style:
We usually prefer C++-style comments // if possible. They are placed in front of variables (not for enums). And you should use doxygen style comments whenever it'll get picked up, so in front of any declaration (class, member, ...). (see also the style stuff here: https://llvm.org/docs/CodingStandards.html)

Test:
It looks like the tests are all positive, e.g., good input results in good return. Could you add some to make sure invalid input is detected _and_ reported as such?

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
92

Why not LOOKUP_PARAMETER(Vector)?

I don't think you want ; at the end.

Cleanup:
#undef LOOKUP_PARAMETER

111

What is the default here? What happens for bad inputs?

121

Doxygen class comment.

122

I found a single use of this exposed member and that use can be replaced by something like:
Call->getModule()
If there is no reason to have this member, get rid of it and all the init stuff.

162

These doxygen documentations do not match the methods.

197

These methods lack doxygen documentation.

217

It seems the new pass manager interface is missing.

llvm/lib/Analysis/SearchVectorFunctionSystem.cpp
278

for (Instruction &I : instructions(F))

aranisumedh added inline comments.Jul 15 2019, 9:50 AM
llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
111

Should I add an assert before the StringSwitch?

Or add OMP_Unknown to handle such cases?

217

Could you share pointers to how to add a new pass using the new pass manager interface?

jdoerfert added inline comments.Jul 15 2019, 11:49 AM
llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
111

I would prefere the OMP_Unknown case but I am probably fine with any error checking solution. I mainly want to avoid silent errors.

217

Take a look at other patches that do it, e.g., D64185

aranisumedh marked 23 inline comments as done.Jul 31 2019, 1:17 PM
aranisumedh added inline comments.
llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
92

Vector is a generic case and not just restricted to OMP. Hence, for the macro to work, there is a prefix OMP_ attached to the rest of the scenarios and not Vector.

122

I did make this change but however, as I'm building the CallInst programmatically in the test case, this leads to a segmentation fault.

However, in the patch that is to follow(hooking to the loopVectorizer), Call->getModule() works perfectly.

Do you have any suggestions as so as to get the getVectorizedFunction test working as specified in SearchVectorFunctionTest.cpp

Also, do I use AssertionSuccess() and AssertionFailure() for testing bad inputs?

Thank you for valuable inputs. Sorry for the delay in getting back to you.

I have mostly used asserts() to capture bad inputs. I have not yet added tests to test for bad input. Google test had documentation using AssertionSuccess() and AssertionFailure(). [https://github.com/google/googletest/blob/master/googletest/docs/advanced.md]. Do I use that or you've something else in mind?

I think someone else should start looking at this.

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
122

You lost me here.

138

I guess you can do this: add a default case, if that value is chosen invoke llvm_unreachable.

llvm/unittests/Analysis/SearchVectorFunctionSystemTest.cpp
274

I doubt we want to output anything here.

aranisumedh marked 3 inline comments as done.Jul 31 2019, 2:41 PM
aranisumedh added inline comments.
llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
122

I have added a new comment that points to the issue better.

138

Will update.

llvm/unittests/Analysis/SearchVectorFunctionSystemTest.cpp
228

I create the CallInst programmatically here. The stack trace tells me the bug is in getVectorizedFunction in SearchVectorFunctionSystem.cpp. When it calls Call->getModule(), a segmentation fault occurs. As I understand, the CallInst is probably not getting inserted in the module.

I was wondering how do I modify the test case so as to suit this functionality?

274

Yes. Forgot to remove the comments. It was for debugging purpose.

Minor changes as suggested in the recent review.

aranisumedh rescinded a token.
aranisumedh marked an inline comment as not done.
jdoerfert added inline comments.Jul 31 2019, 3:16 PM
llvm/unittests/Analysis/SearchVectorFunctionSystemTest.cpp
228

CallInst should have a constructor/create method that takes a basic block or instruction. Alternatively, call Call->insertXXX later.

Thanks for working on this! I'm really interested in using this functionality so here are some comments on the main interface header.

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
45

How is this different from Vector?

56

That's great, I see how it can be used for non-OMP usage.

61

Is it original (scalar) call position or vector call position? Please make it clear in the comment.

I'm interested in this because my use case might have Mask argument in the vector function variant in the middle of other arguments. Is it possible to handle that with this patch?

66

My preference for this is to make the ctor private and provide public static functions to create different set of required arguments for different kinds. E.g. no LinearStepOrPos of varying/vector parameter. SOmething like this:

private:
  ParamType()

public
  static ParamType vector() { return ParamType{}; }
  static ParamType linear(int LinearStep) { ... }
  static ParamType linearWithNonConstStride(unsigned StepArgPosition) { ... }

Feel free to ignore though.

84

I'm not sure if more broad "isLinear" interface for all the Linear* would be useful. Kind of sad if we won't be able to have it because of this accessors.

94

I would split it into two methods with proper asserts for the correct kind of this param.

96

Do we really need the setter? Why once created param description could change?

121

This is marked as done but I don't see the doxygen comment...

131

Isn't it part of ISA somehow? Why do we need a separate flag that isn't applicable for all the ISAs? Or do I miss something here?

Also, I'm not familiar with is (I assume it's related to SVE) - how does it deal with VF. Do we still need VF here if IsScalable is true?

134

Is it possible to add the information about the return value? Like varying vector/uniform scalar/uniform vector.

171

I'd rather have an interface not referencing the CallInst (e.g. mangled name instead). Why doesn't it have a "bool Masked" parameter?

178

Same as above, I think. E.g. interface without CallInst, bool Mask.

195

I'm not sure why is in VFABI namespace. Mask/NoMask isn't ABI-specific. Same for functions below.

aranisumedh added inline comments.Aug 1 2019, 12:34 PM
llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
45

You mean Unknown? It's for error handling.

61

It is the original call position.

84

It just checks if the parameter type is OMP_Linear. It doesn't check for Linear* case

94

According to the VFABI, it'll be either step or pos associated. If we split this into two, we would need more variables.

As per the ABI, if it's ls, Rs, Ls, Us, u, only then would we pos associated with the parameter else it's a step.

96

No. I don't think the parameters can change. I can change the implementation so as to avoid using the setter function.

131

If the VF is set to 'x' and and the ISA is SVE, it is then the IsScalable flag is set to true.

134

Could you elaborate, so as to make sure I get you right?

171

We are trying to search for all the vector function mappings available for a given scalar function by querying the SVFS table. Hence, I think that it is better to pass the CallInst, where we check for records in our table with matching scalar names. It also follows the description in the RFC.

178

You can specify the bool Mask within the VectorFunctionShape that is the second parameter. In the separate patch that is to follow, I'll demonstrate how the loop vectorizer will call and query SVFS to vectorize a CallInst.

195

The parsing of the mangled names is as described in the Vector Function ABI. All these functions take in mangled name and parse the information. Hence, I decided to keep these function within the VFABI namespace.

Thanks @aranisumedh for working on this!

The patch is quite big and I think it would be easier to review if it is split up. My main concern is that this patch conflates the SVFS pass/mechanism and the (demangling for the) AArch64 Vector ABI, which I think need to be split up into separate patches.

If you add a print method to the SVFS pass, you can print the available vector mappings as described in LLVM IR function attributes by running opt -analyze -svfs, which would make it easier to test using FileCheck. If you add that first, you could use the -analyze functionality to test the demangling of the AArch64 Vector ABI with .ll tests.

Not sure if this was already discussed anywhere else, but is there a reason this doesn't live in TargetLibraryInfo? There is already a getVectorizedFunction in that class, so it makes sense to make that method more powerful by having it take a VectorFunctionShape as well as unsigned VF.

llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
45

It is more explicit to rename it to Invalid in that case.

81

Can you just expand these functions instead of using a pragma? Given the simplicity of the definition there is little benefit to using a macro here, and having them expanded would make it easier to search for in the codebase.

100

is this not specific to the AArch64 Vector ABI?

128

VectorFunctionShape could benefit from having a constructor, if only to ensure that a VectorFunctionShape is valid and can be used as operand in operator==.

153

Do we want to expose RecordTable?

179

This method asks for a vectorized function that *exactly*matches the specifications in VectorFunctionShape info. You may want to think about a mechanism in VectorFunctionShape where you can distinguish "required" and "optional" features. For example, if this function is asked for to return an unpredicated vector function, but it only has a predicated vector function, we'd still prefer to get the predicated vector function and construct an "all true" predicate, rather than having to fall back on scalar instructions.

llvm/lib/Analysis/SearchVectorFunctionSystem.cpp
266

I think you should either support 1 mapping, or support multiple mappings with a clear mechanism to prioritise which mapping takes priority. This function seems to prioritise based on whether or not a function-name is mangled which makes little sense to me.

aranisumedh planned changes to this revision.Aug 6 2019, 8:43 AM

As suggested earlier by Sander, I'll be splitting this patch into two patches for easier review.

aranisumedh marked 15 inline comments as done.Aug 6 2019, 11:06 AM
aranisumedh added inline comments.
llvm/include/llvm/Analysis/SearchVectorFunctionSystem.h
96

The current implementation allows me to handle corner cases better. This setter function is used to set the LinearStepOrPos for the first time.

100

No. IIRC, this is in agreement with other vendors too.

121

That is because it was a part of the last diff. Hence, the lines get moved. The getter and setters are pretty straightforward. Do I still add a doxygen comment for the same?

153

Making it private.

aranisumedh marked 8 inline comments as done.Aug 9 2019, 12:20 PM
aranisumedh added inline comments.
llvm/lib/Analysis/SearchVectorFunctionSystem.cpp
266

The priority is given on the basis that there exists a redirection to a custom user-defined function. This scenario arises from the different uses of declare simd and declare variant.

And the implementation follows that we save the VectorName as the mangledName if there is no redirection. Hence we prioritize declare variant over declare simd