[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:2451
   for (const auto &N : VectorizableTree) {
-    Instruction *Inst = dyn_cast<Instruction>(N.Scalars[0]);
+    Instruction *Inst = dyn_cast<Instruction>(N.State.OpValue);
     if (!Inst)
----------------
`auto *Inst`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2939
 
-  InstructionsState S = getSameOpcode(E->Scalars);
-  Instruction *VL0 = cast<Instruction>(E->Scalars[0]);
+  Instruction *VL0 = cast<Instruction>(E->State.OpValue);
   Type *ScalarTy = VL0->getType();
----------------
`auto *VL0`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3613
+    for (Value *V : VL) {
+      Instruction *I = dyn_cast<Instruction>(V);
+      if (!I)
----------------
`auto *I`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3621
+      if (I->getOpcode() == Instruction::ExtractElement ||
+          I->getOpcode() == Instruction::ExtractValue)
+        for (Value *V1 : VL) {
----------------
Why you're checking for `ExtractValue`s here?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3623
+        for (Value *V1 : VL) {
+          Instruction *I1 = dyn_cast<Instruction>(V1);
+          if (I1 != nullptr && I1->getOpcode() != Instruction::ExtractElement &&
----------------
`auto *I1`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3624
+          Instruction *I1 = dyn_cast<Instruction>(V1);
+          if (I1 != nullptr && I1->getOpcode() != Instruction::ExtractElement &&
+              I1->getOpcode() != Instruction::ExtractValue &&
----------------
1. `I1 != nullptr` -> `I1`
2. `I1` = vis very bad name


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3629
+        }
+      // Check inner vector dependencies
+      for (User *U : I->users()) {
----------------
I still don't understand what are checking for here. Need more description and real tests


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3631
+      for (User *U : I->users()) {
+        Instruction *UserInst = dyn_cast<Instruction>(U);
+        if (!UserInst)
----------------
`auto *UserInst`


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3635
+        for (Value *V : VL) {
+          Instruction *I1 = dyn_cast<Instruction>(V);
+          if (!I1)
----------------
1. `auto *I1`
2. `I1` is very bad name


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4074
+        LastScheduledInst = pickedInst;
+      } else NonAlters.insert(pickedInst);
       BundleMember = BundleMember->NextInBundle;
----------------
1. Wrong formatting. Use `clang-format`
2. Enclose into braces


================
Comment at: test/Transforms/SLPVectorizer/X86/pr35497.ll:1
+; RUN: opt -slp-vectorizer -S -mtriple=x86_64-unknown-linux-gnu < %s
+
----------------
Add test checks


https://reviews.llvm.org/D28907