[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:1483
+        // dependency and non alternative opcode.
+        if ((Entry->State.IsNonAlt) &&
+            !getTreeEntry(U) && getTreeEntry(U, Scalar) == Entry)
----------------
1. Remove extra parens around `Entry->State.IsNonAlt`
2. Why are you skipping bundles with non alternarive opcodes only?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3007-3011
+      ScheduleData *First = Bundle->FirstInBundle;
+      SmallPtrSet<Instruction *, 4> BundleInst;
+      for (Bundle = First; Bundle; Bundle = Bundle->NextInBundle)
+        BundleInst.insert(Bundle->Inst);
+      LastInst = First->Inst;
----------------
Rewrite it this way:
```
SmallPtrSet<Instruction *, 4> BundleInst;
Bundle = Bundle->FirstInBundle;
LastInst = Bundle->Inst;
while (Bundle) {
  BundleInst.insert(Bundle->Inst);
  Bundle = Bundle->NextInBundle;
}
```


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3012-3014
+      for (BasicBlock::iterator Iter = First->Inst->getIterator(),
+                                LowerEnd = BB->end();
+           (Iter != LowerEnd && !BundleInst.empty());) {
----------------
Why do you need to scan all the instructions in the basic block starting from `First`? Why you can't use only scheduled instructions?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3015
+           (Iter != LowerEnd && !BundleInst.empty());) {
+        Instruction *Inst = &*Iter++;
+        if (BundleInst.count(Inst)) {
----------------
Move `++Iter` to the header of `for` loop


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3811
+      // dependency and non alternative opcode.
+      bool skip = false;
+      if (Entry->State.IsNonAlt)
----------------
Name of the variable must start from capital letter.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3814
+        for (User *U : Scalar->users()) {
+          if (!getTreeEntry(U) && getTreeEntry(U, Scalar) == Entry)
+          skip = true;
----------------
1. The code is not formatted
2. Seems to me you missed `break;`


https://reviews.llvm.org/D28907