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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added a comment.

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



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:3031
+      VFShape Shape =
+          VFShape::get(*CI, {static_cast<unsigned int>(VL.size()), false},
+                       false /*HasGlobalPred*/);
----------------
Please describe the `false` parameter like you have done for `/*HasGlobalPred*/`.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:4545
+      } else {
+        Module *M = F->getParent();
+        Type *Tys[] = {FixedVectorType::get(CI->getType(), E->Scalars.size())};
----------------
`M` is used only inside `getDeclaration`, no need to declare a variable for it.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5110
+
+  return true;
+}
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:5208
+          bool SrcMayWrite = BundleMember->Inst->mayWriteToMemory() &&
+                             !isVectorizableLibFunctionCall(BundleMember->Inst);
           unsigned numAliased = 0;
----------------
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.


================
Comment at: llvm/test/Transforms/SLPVectorizer/AArch64/accelerate-vector-functions.ll:66
 ; CHECK-NEXT:    [[TMP0:%.*]] = load <4 x float>, <4 x float>* [[A:%.*]], align 16
-; CHECK-NEXT:    [[VECEXT:%.*]] = extractelement <4 x float> [[TMP0]], i32 0
-; CHECK-NEXT:    [[TMP1:%.*]] = tail call fast float @ceilf(float [[VECEXT]]) #1
-; CHECK-NEXT:    [[VECINS:%.*]] = insertelement <4 x float> undef, float [[TMP1]], i32 0
-; CHECK-NEXT:    [[VECEXT_1:%.*]] = extractelement <4 x float> [[TMP0]], i32 1
-; CHECK-NEXT:    [[TMP2:%.*]] = tail call fast float @ceilf(float [[VECEXT_1]]) #1
-; CHECK-NEXT:    [[VECINS_1:%.*]] = insertelement <4 x float> [[VECINS]], float [[TMP2]], i32 1
-; CHECK-NEXT:    [[VECEXT_2:%.*]] = extractelement <4 x float> [[TMP0]], i32 2
-; CHECK-NEXT:    [[TMP3:%.*]] = tail call fast float @ceilf(float [[VECEXT_2]]) #1
-; CHECK-NEXT:    [[VECINS_2:%.*]] = insertelement <4 x float> [[VECINS_1]], float [[TMP3]], i32 2
-; CHECK-NEXT:    [[VECEXT_3:%.*]] = extractelement <4 x float> [[TMP0]], i32 3
-; CHECK-NEXT:    [[TMP4:%.*]] = tail call fast float @ceilf(float [[VECEXT_3]]) #1
-; CHECK-NEXT:    [[VECINS_3:%.*]] = insertelement <4 x float> [[VECINS_2]], float [[TMP4]], i32 3
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast <4 x float> @vceilf(<4 x float> [[TMP0]])
+; CHECK-NEXT:    [[TMP2:%.*]] = extractelement <4 x float> [[TMP1]], i32 0
----------------
Nice! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82550