[PATCH] D57059: [SLP] Initial support for the vectorization of the non-power-of-2 vectors.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

Some initial comments - there's a lot going on (cutting it into smaller patches would be very useful).

The sdiv undefs test worries me, and I'm not certain if we have good masked load/store costs at the moment.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1500
+          Value *UseScalar =
+              *llvm::find_if(UseEntry->Scalars, Instruction::classof);
           // Some in-tree scalars will remain as scalar in vectorized
----------------
Check for failure instead of just dereferencing?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2008
+          continue;
+        auto *Op = cast<Instruction>(V)->getOperand(1);
         if (!isa<ConstantInt>(Op)) {
----------------
drop the * ?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2138
+        for (Value *V : VL) {
+          auto *CI2 = dyn_cast<CallInst>(V);
           Operands.push_back(CI2->getArgOperand(i));
----------------
drop the * ?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2298
+  Value *V0;
   Type *ScalarTy = VL[0]->getType();
+  VectorType *VecTy;
----------------
Is it safe to use VL[0] like this - do we guarantee the undef types are correct?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2987
                                   bool AllSameOpcodeRight, bool SplatLeft,
                                   bool SplatRight) {
+  Value *VLeft = I.getOperand(0);
----------------
This is going to cause problems for an incoming patch I'm working on - any chance that you can avoid using Instruction &I?


================
Comment at: test/Transforms/SLPVectorizer/AArch64/PR38339.ll:16
+; CHECK-NEXT:    store i16 [[T3]], i16* [[PTR2]]
+; CHECK-NEXT:    store i16 [[T2]], i16* [[PTR3]]
 ; CHECK-NEXT:    ret void
----------------
regression?


================
Comment at: test/Transforms/SLPVectorizer/X86/addsub.ll:319
+; CHECK-NEXT:    [[TMP14:%.*]] = getelementptr inbounds double, double* [[C]], i64 1
+; CHECK-NEXT:    store double [[TMP13]], double* [[TMP14]]
 ; CHECK-NEXT:    ret void
----------------
regression?


================
Comment at: test/Transforms/SLPVectorizer/X86/alternate-fp.ll:87
+; SLM-NEXT:    [[R6:%.*]] = insertelement <8 x float> [[R5]], float [[AB6]], i32 6
+; SLM-NEXT:    [[R7:%.*]] = insertelement <8 x float> [[R6]], float [[AB7]], i32 7
 ; SLM-NEXT:    ret <8 x float> [[R7]]
----------------
regression?


================
Comment at: test/Transforms/SLPVectorizer/X86/alternate-int.ll:590
+; AVX512-LABEL: @sdiv_v8i32_undefs(
+; AVX512-NEXT:    ret <8 x i32> undef
 ;
----------------
This test is checking that the sdiv didn't get vectorized, because if any elt is undef then the whole result is undef - but if its stays scalarized then on elts 0 and 4 become undef.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57059