[PATCH] D28961: [SLP] Fix for PR31690: Allow using of non-zero initial values in horizontal instructions.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4256
+        // Check if the NextV argument can be used later after vectorization.
+        if ((EdgeToVist == 0 || TreeN->isAssociative()) &&
+            !ExtraArgs.count(TreeN)) {
----------------
mkuper wrote:
> I don't understand the logic here.
> IIUC, if EdgeToVist is 0 it just means that we're currently going "left", but this could be at any stage in the reduction. Why does EdgeToVist == 0 means we can do this for a non-associative FAdd?
Removed it, now there is only check for associative operation


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4329
+        if (auto *I = dyn_cast<Instruction>(V))
+          Builder.SetCurrentDebugLocation(I->getDebugLoc());
+        VectorizedTree = createBinOp(Builder, ReductionOpcode, VectorizedTree,
----------------
mkuper wrote:
> Don't you want the debugloc for the original op (Pair.first?), not for its operand?
Good catch, thanks.


================
Comment at: test/Transforms/SLPVectorizer/X86/horizontal-list.ll:748
   ret float %add.31
 }
 
----------------
mkuper wrote:
> Could you please add a test that has several "extra" values?
> It doesn't have to have so many vector lanes, that's just noise.
Will do


https://reviews.llvm.org/D28961