[PATCH] D59881: Initial support for vectorization using MASSV (IBM MASS vector library)

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/include/llvm/Analysis/VecFuncs.def:76
+TLI_DEFINE_VECFUNC("llvm.exp.f64", "__expd2_massv", 2)
+TLI_DEFINE_VECFUNC("expf", "__expf4_massv", 4)
+TLI_DEFINE_VECFUNC("llvm.exp.f32", "__expf4_massv", 4)
----------------
How about the finite versions? eg: __expf_finite


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll:1
+; RUN: opt -vector-library=MASSV -loop-vectorize -force-vector-interleave=1 -S < %s | FileCheck %s
+
----------------
do we require any attr? eg: vsx? what happen to ppc without altivec or vsx?


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll:4
+target datalayout = "e-m:e-i64:64-n32:64" 
+target triple = "powerpc64le-unknown-linux-gnu"
+
----------------
How about powerpc (32 bit) or powerpc64?


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll:93
+; CHECK-LABEL: @cbrt_f64(
+; CHECK: __cbrtd2_massv{{.*}}<2 x double>
+; CHECK: ret void
----------------
It would be better if we check all the key elements of call , not only the name .
eg: call <2 x double> @__cbrtd2_massv(<2 x double> %2)


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll:282
+}
+
+define void @exp_f64(double* nocapture %varray) {
----------------
Missing some intrinsics test? @sqrt_f64_intrinsic?@sqrt_f32_intrinsic?


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll:1524
+}
+
+attributes #0 = { nounwind }
----------------
Shall we add some negative tests to make sure that we don't vectorize functions not in the list, eg: arbitrary functions, function in ACCELERATE/SVML but not in MASSV?


================
Comment at: llvm/test/Transforms/LoopVectorize/PowerPC/massv-calls.ll:1525
+
+attributes #0 = { nounwind }
+
----------------
Maybe add a negative test where the nobuiltin attribute is present too.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59881