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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper 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)) {
----------------
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?


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


================
Comment at: test/Transforms/SLPVectorizer/X86/horizontal-list.ll:748
   ret float %add.31
 }
 
----------------
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.


https://reviews.llvm.org/D28961