[PATCH] D28907: [SLP] Fix for PR30787: Failure to beneficially vectorize 'copyable' elements in integer binary ops.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few minor comments.



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:409
       return {};
-    if (Opcode != I->getOpcode())
+    if (!sameOpcodeOrAlt(Opcode, AltOpcode, I->getOpcode())) {
+      if (unsigned NewOpcode = tryToRepresentAsInstArg(Opcode, I)) {
----------------
You can probably tidy this up by doing an early-out:

```
if (sameOpcodeOrAlt(Opcode, AltOpcode, I->getOpcode()))
  continue;
```


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1582
   // schedulable.
-  Instruction *VL0 = cast<Instruction>(S.OpValue);
+  auto *VL0 = cast<Instruction>(S.OpValue);
   BasicBlock *BB = VL0->getParent();
----------------
NFC pre-commit


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1843
+            Operand = I->getOperand(i);
+          Operands.push_back(Operand);
+        }
----------------
Possible tidyup:
```
if (I->getOpcode() == S.Opcode) {
  Operands.push_back(I->getOperand(i));
  continue;
}
assert(Instruction::isBinaryOp(S.Opcode) && "Expected a binary operation.");
Value *Operand = isOdd(i) ? getDefaultConstantForOpcode(S.Opcode, I->getType())
                                                : VecOp;
Operands.push_back(Operand);
```


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2018
+            Operand = I->getOperand(i);
+          Operands.push_back(Operand);
+        }
----------------
Tidyup similar to above?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2260
+      } else
+        Op2VK = TargetTransformInfo::OK_AnyValue;
 
----------------
This setting of Op2VK is a bit confusing with dangling if-else cases - why not set to OK_AnyValue by default and try to make it OK_UniformConstantValue (maybe just at the start of the 'if (auto *CInt = dyn_cast<ConstantInt>(VL0->getOperand(1))) {') ?


https://reviews.llvm.org/D28907