[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:1483
+        // dependency and non alternative opcode.
+        if ((Entry->State.IsNonAlt) &&
+            !getTreeEntry(U) && getTreeEntry(U, Scalar) == Entry)
----------------
ABataev wrote:
> 1. Remove extra parens around `Entry->State.IsNonAlt`
> 2. Why are you skipping bundles with non alternarive opcodes only?
yes, we don't need to check Entry->State.IsNonAlt here, that is similar to getTreeEntry(U, Scalar) == Entry.


================
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;
----------------
ABataev wrote:
> Rewrite it this way:
> ```
> SmallPtrSet<Instruction *, 4> BundleInst;
> Bundle = Bundle->FirstInBundle;
> LastInst = Bundle->Inst;
> while (Bundle) {
>   BundleInst.insert(Bundle->Inst);
>   Bundle = Bundle->NextInBundle;
> }
> ```
Done. Thanks.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3012-3014
+      for (BasicBlock::iterator Iter = First->Inst->getIterator(),
+                                LowerEnd = BB->end();
+           (Iter != LowerEnd && !BundleInst.empty());) {
----------------
ABataev wrote:
> Why do you need to scan all the instructions in the basic block starting from `First`? Why you can't use only scheduled instructions?
What do you mean? please elaborate. If you mean ScheduleData here then it is also not always gives up correct sequence of scheduled instructions in a block. Like, bundle member with NextInBundle equals to null is not guaranty to be the last instruction of this bundle among other instructions. If we start with First then it is highly likely that we could iterate across all bundle members and exit instead of iterating to the end of the basic block. Also, I am thinking, to avoid this overhead we could note the last scheduled instruction in BB during scheduling in scheduleBlock() and keep this information in ScheduleData structure.


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


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


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


https://reviews.llvm.org/D28907