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


================
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();
----------------
ABataev wrote:
> `auto *VL0`
done.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3621
+      if (I->getOpcode() == Instruction::ExtractElement ||
+          I->getOpcode() == Instruction::ExtractValue)
+        for (Value *V1 : VL) {
----------------
ABataev wrote:
> Why you're checking for `ExtractValue`s here?
done. I don't have any limitation for the extract element operation anymore.


================
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 &&
----------------
ABataev wrote:
> `auto *I1`
done.


================
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 &&
----------------
ABataev wrote:
> 1. `I1 != nullptr` -> `I1`
> 2. `I1` = vis very bad name
done.


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


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


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


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


https://reviews.llvm.org/D28907