[PATCH] D29727: [SLP] Fix for PR31690: Allow using of extra values in horizontal reductions.

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added a comment.

Oh, ok, I see what's going on now. I've misread the test result, and didn't notice how it's being treated. Sorry for the noise.
(It would have been easier, though, if you updated the patch description and/or added documentation to the original patch, instead of just reusing the description of the original commit).

Some minor comments inline.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2856
     Value *Lane = Builder.getInt32(ExternalUse.Lane);
+    // If User == nullptr, the Scalar is used as extra arg. Generate
+    // ExtractElement instruction and update the record for this scalar in
----------------
Please also change the documentation of ExternalUses to explain that a nullptr "User" is legal, and what it means (Line 589).


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2860
+    if (!User) {
+      assert(ExternallyUsedValues.count(Scalar));
+      DebugLoc DL = ExternallyUsedValues[Scalar];
----------------
Add a meaningful message to the assert.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2862
+      DebugLoc DL = ExternallyUsedValues[Scalar];
+      if (auto *VecI = dyn_cast<Instruction>(Vec)) {
+        Builder.SetInsertPoint(VecI->getParent(),
----------------
I guess it's impossible for Vec to be a PHI here, because the real user is going to be a reduction operation, right?
If so, how is it possible for it to be a non-Instruction?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2865
+                               std::next(VecI->getIterator()));
+      } else
+        Builder.SetInsertPoint(&F->getEntryBlock().front());
----------------
I'd say either have no braces, or braces for both the if and the else block - this looks a bit weird.


https://reviews.llvm.org/D29727