[PATCH] D82550: [SLPVectorizer] handle vectorized lib functions

[Sanne Wouda via Phabricator via llvm-commits]

sanwou01 added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5208
+          bool SrcMayWrite = BundleMember->Inst->mayWriteToMemory() &&
+                             !isVectorizableLibFunctionCall(BundleMember->Inst);
           unsigned numAliased = 0;
----------------
ABataev wrote:
> fpetrogalli wrote:
> > ABataev wrote:
> > > fpetrogalli wrote:
> > > > ABataev wrote:
> > > > > 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 @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.
> > > 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.
> > 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?
> Maybe, `nosync` attribute?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82550