[PATCH] D28907: [SLP] Fix for PR30787: Failure to beneficially vectorize 'copyable' elements in integer binary ops.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3629
+        }
+      // Check inner vector dependencies
+      for (User *U : I->users()) {
----------------
dtemirbulatov wrote:
> ABataev wrote:
> > I still don't understand what are checking for here. Need more description and real tests
> I hit the issue without "(SameOrAlt <= (VL.size() / 2))" limitation, I can send you testcases offline. but I believe with could trigger the issue without this check even in the current implementation.
The check itself is very expensive and not clear.That's why it causes a lot of questions. You should describe the problem in more details, add the test case to the patch that reveals the problem and prove, that this fix is universal.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2207
     case Instruction::ExtractElement:
-      if (canReuseExtract(VL, S.OpValue)) {
+      if (canReuseExtract(VL, E->State.OpValue)) {
         int DeadCost = 0;
----------------
You can use `VL0` instead of `E->State.OpValue`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3610
+                                                 const InstructionsState &S,
+                                                 DominatorTree *DT) {
+  if (isa<PHINode>(S.OpValue))
----------------
`DominatorTree *DT` is not required.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3657
+  for (Value *V : VL)
+    if (!extendSchedulingRegion(V, S.OpValue))
       return false;
----------------
Seems to me, we should extend scheduling region only for instructions with the same or alternate opcode. Maybe, it will resolve all your problems.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4079
+          LastScheduledInst = pickedInst;
+          std::set<Instruction *> Reschedule;
+          Reschedule.insert(pickedInst);
----------------
1. Use `SmallSet` instead
2. Why you can't use `SmallVector` instead?


https://reviews.llvm.org/D28907