[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 a comment.

Also, your code seems not quite formatted. Please, use clang-format on your changes to format it properly.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:433
+                                       cast<Instruction>(VL[NewOpcodePos])))
+      Res.NeedAnalysis = true;
+    else
----------------
Enclose it into braces


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:435
+    else
+      return {};
   }
----------------
This too


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:474
   unsigned AltOpcode = getAltOpcode(Opcode);
+  bool IsNonAlt = false;
+  for (Value *V : VL) {
----------------
```
bool IsNonAlt = llvm::one_of(VL, [Opcode, AltOpcode](Value *V) {return isa<Instruction>(V) && !sameOpcodeOrAlt(Opcode, AltOpcode, cast<Instruction>(V)->getOpcode());});
```


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1616
+    if (SameOrAlt <= (VL.size() / 2)) {
+      newTreeEntry(VL, false, UserTreeIdx, S);
+      return;
----------------
Add the debug message here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3112
+      int MaxDist = 0;
+      while (Bundle) {
+        int Dist = std::distance(BB->begin(), Bundle->Inst->getIterator());
----------------
Why you can't put bundles in the list in the right order: from the very first instruction to the very last?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3938-3945
+        for (User *U : Scalar->users()) {
+          if (!getTreeEntry(U) && getTreeEntry(U, Scalar) == Entry) {
+            Skip = true;
+            break;
+          }
+        }
+      if (Skip)
----------------
Better to do it this way:
```
if (llvm::any_of(Scalar->users(), [this, Entry, Scalar](User *U){return !getTreeEntry(U) && getTreeEntry(U, Scalar) == Entry;}))
  continue;
```


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4481
+    // regular opertions in order to stay within a bundle boundaries.
+    for (Instruction *pickedInst : NonAlters)
       if (LastScheduledInst->getNextNode() != pickedInst) {
----------------
`pickedInst`->`PickedInst`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4486
                                      pickedInst);
+        Value *V = dyn_cast<Value>(pickedInst);
+        ScheduleData *SD = BS->getScheduleData(V);
----------------
Remove this, it is not needed


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4500-4502
+            auto *OpValue = dyn_cast<Value>(I);
+            if (!OpValue)
+              continue;
----------------
Remove it, does not do anything


https://reviews.llvm.org/D28907