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

[Dinar Temirbulatov via Phabricator via llvm-commits]

dtemirbulatov added inline comments.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3629
+        }
+      // Check inner vector dependencies
+      for (User *U : I->users()) {
----------------
ABataev wrote:
> 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.
Added testcase internal-dep.ll for that, the issue could be reproduced if we remove this check and remove another "(SameOrAlt <= (VL.size() / 2))" limitation at line 1556. I believe that the issue is present even in the current implementation, but I just don't have enough code base to prove it.


================
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;
----------------
ABataev wrote:
> You can use `VL0` instead of `E->State.OpValue`
done.


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


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4079
+          LastScheduledInst = pickedInst;
+          std::set<Instruction *> Reschedule;
+          Reschedule.insert(pickedInst);
----------------
ABataev wrote:
> 1. Use `SmallSet` instead
> 2. Why you can't use `SmallVector` instead?
Replaced to SmallSet, We could not use Vector here since it is not possible to have multiple entities to a single Instruction.


https://reviews.llvm.org/D28907