This is an archive of the discontinued LLVM Phabricator instance.

[SLPVectorizer] handle vectorized lib functions
ClosedPublic

Authored by sanwou01 on Jun 25 2020, 7:26 AM.

Details

Summary

Teaches the SLPVectorizer to use vectorized library functions for
non-intrinsic calls.

This already worked for intrinsics that have vectorized library
functions, thanks to D75878, but schedules with library functions with a
vector variant were being rejected early.

  • assume that there are no load/store dependencies between lib functions with a vector variant; this would otherwise prevent the bundle from becoming "ready"
  • check during legalization that the vector variant can be used
  • fix-up where we previously assumed that a call would be an intrinsic

Diff Detail

Event Timeline

sanwou01 created this revision.Jun 25 2020, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2020, 7:26 AM
fhahn added a comment.Jun 25 2020, 7:31 AM

Thanks for the patch. Some comments inline

llvm/include/llvm/Analysis/VectorUtils.h
227 ↗(On Diff #273348)

unrelated to the change? If so, better to split it off (and perhaps add a test independent of SLP?)

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3036

please run clang format-diff on the patch,

3062

Please strip the unrelated whitespace changes.

5092

This is a NFC change, right? Would be good to split off and commit separately. Looks like an independent improvement.

llvm/test/Transforms/SLPVectorizer/AArch64/accelerate-vector-functions.ll
16 ↗(On Diff #273348)

It would be good to pre-commit the test and then only have the diff of the changes here.

ABataev added inline comments.Jun 25 2020, 7:33 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3035–3036

Formatting

llvm/test/Transforms/SLPVectorizer/AArch64/accelerate-vector-functions.ll
12 ↗(On Diff #273348)

Pre-commit the new tests with the current results to see the changes

sanwou01 updated this revision to Diff 273383.Jun 25 2020, 8:29 AM
sanwou01 marked 8 inline comments as done.

Addressed comments

llvm/include/llvm/Analysis/VectorUtils.h
227 ↗(On Diff #273348)

The underlying problem here is that if getVFABIMappings is called with an indirect call instruction, it'll call getName() on a nullptr. I suspect other callers take care not to do that, making it a latent bug without the rest of this patch.

Agreed though that it deserves its own patch and test; not sure if I can write a test independent of SLP though.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5092

I've split the NFC bit off and will commit that first.

Ah, I missed the test changes this time round. Incoming.

sanwou01 updated this revision to Diff 273385.Jun 25 2020, 8:31 AM

Now with test changes

Harbormaster failed remote builds in B61748: Diff 273383!
Harbormaster failed remote builds in B61749: Diff 273385!
RKSimon added inline comments.Jul 3 2020, 3:57 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3042

NFC rename?

4548

This looks like a separate NFC?

sanwou01 marked 2 inline comments as done.Jul 3 2020, 4:29 AM
sanwou01 added inline comments.
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3042

Sure, I can split that off

4548

Not sure if I'd call this NFC. It fixes a crash when !UseIntrinsic and there is no intrinsic, only a LibFunc, for the call. This patch makes that condition possible here.

Could do as a separate patch though, if that makes sense.

sanwou01 updated this revision to Diff 275365.Jul 3 2020, 4:40 AM

Split out NFC rename

sanwou01 marked an inline comment as not done.Jul 3 2020, 4:50 AM

@ABataev any more comments?

@ABataev any more comments?

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3030

Is this a pointer? Then auto *.

5121

Why do you need to exclude vectorizable library functions here?

sanwou01 marked 2 inline comments as done.Jul 6 2020, 3:31 AM

Comments inline.

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3030

Surprisingly no! I'll remove auto to avoid the confusion.

5121

For "normal" function calls, we have to assume that the functions may read or write memory any location in memory, which may alias memory read or written by another instruction in the same bundle. For functions with vector variants, we should be able to assume that they are pure: they won't write to memory (except when the function takes pointer arguments, which I'm not handling correctly now that I think about it; I'll fix that up).

Actually, the calculateDependencies function below might be the less-surprising place to handle this.

ABataev added inline comments.Jul 6 2020, 5:34 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5121

Then it is better to add correct attributes to such functions. Special processing may lead to problems later.

sanwou01 updated this revision to Diff 275712.Jul 6 2020, 7:47 AM
sanwou01 marked an inline comment as done.

Addressed comments.

  • Moved handling of vector lib memory dependencies to calculateDependencies
  • Fixed memory dependencies of library functions that take pointers as arguments
ABataev added inline comments.Jul 6 2020, 7:52 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5200

Still, this is a bad solution. Add proper attributes to the vector variants of the functions, so all memory access interfaces could properly handle such functions.

Hi @sanwou01 , thank you for working on this!

I left a couple of comments.

Additional question: you seem to be testing only math functions. But it seems that your code would be working with any function that has the vector function abi variant attribute attached. Mmight be worth adding a test for a generic function?

Kind regards,

Francesco

llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
3031

Please describe the false parameter like you have done for /*HasGlobalPred*/.

4545

M is used only inside getDeclaration, no need to declare a variable for it.

5110

I suspect there are many things that may fail here, other than not having a mapping in VFDatabase or having pointer arguments. I think it would be safer to reverse the logic, and have the function return false by default, and return true if VFDatabase is not empty and there is no pointer arguments.

5200

HI @ABataev, if I understood correctly, you are asking to add a new attribute that guaranteed that the function does not have side effects? If that's the case, that is already guaranteed by the vector-function-abi-variant attribute.

llvm/test/Transforms/SLPVectorizer/AArch64/accelerate-vector-functions.ll
66 ↗(On Diff #275712)

Nice! :)

ABataev added inline comments.Jul 6 2020, 8:36 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5200

Hi. No, what I'm asking is to mark the vectorized function versions with attributes that they don't write to the memory (what this change implies), so all attribute interfaces properly handle such functions as no-write functions.

fpetrogalli added inline comments.Jul 6 2020, 9:03 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5200

I just had a chat with @sanwou01 , we agreed that it is better to use the VFABI variant attribute just to describe the mapping, and have other attributes (like readonly) attached to the function to guarantee the fact that there is not side effects.

@sanwou01 is coming up with a unit test in which a function is marked with both the readonly and vector-function-abi-variant. Is there any other attribute he should add to guarantee that the memory accesses are compatible with a concurrent invocation of the function?

ABataev added inline comments.Jul 6 2020, 9:08 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5200

Maybe, nosync attribute?

sanwou01 added inline comments.Jul 6 2020, 9:36 AM
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
5200

Agreed that relying on the proper attributes is much cleaner.

nosync is too weak, it only guarantees no communication between threads e.g. through the absence of volatile, not between subsequent calls on the main thread.

readonly would be sufficient for most math functions. argmemonly would be the right attribute for functions that take pointers (possibly with readonly), but we'd have to check aliasing in that case.

Also note that the attribute has to be set on the original scalar function (that's what's being checked here); the vector variant, by being an implementation of the scalar function, should also conform to the attribute.

sanwou01 updated this revision to Diff 277055.Jul 10 2020, 8:34 AM
sanwou01 marked 3 inline comments as done.

Updates to address feedback, in particular:

  • use readonly attribute to determine memory dependencies between call instructions, and update tests accordingly
  • add a standalone test that does not rely on -vector-library
llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
4545

I'm just moving some pre-existing code into an else clause here. I can still tidy this up if you prefer, but perhaps in a follow-up NFC?

5110

As discussed below, we can rely on the existing readonly attribute here, so this helper function is no longer needed, and removed.

This revision is now accepted and ready to land.Jul 10 2020, 9:31 AM
This revision was automatically updated to reflect the committed changes.