[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:304
+  return (Opcode == Instruction::SRem || Opcode == Instruction::URem ||
+          Opcode == Instruction::SRem || Opcode == Instruction::FRem);
+}
----------------
Still 2 `SRem`s


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:387
 /// OpValue.
-static Value *isOneOf(const InstructionsState &S, Value *Op) {
-  auto *I = dyn_cast<Instruction>(Op);
+static Value *isOneOf(const InstructionsState &S, Instruction *I) {
   if (I && S.isOpcodeOrAlt(I))
----------------
Restore the original function here.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:408
   unsigned AltOpcode = Opcode;
+  unsigned OpcodeNum = 0;
+  unsigned AltOpcodeNum = 0;
----------------
You definitely need comments here


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:812
   /// Create a new VectorizableTree entry.
   void newTreeEntry(ArrayRef<Value *> VL, bool Vectorized, int &UserTreeIdx,
+                    const InstructionsState &S,
----------------
Instead of `ArrayRef<Value *>` I expected to see something like `ArrayRef<InstructionOrPseudoInstruction>`, where `InstructionOrPseudoInstruction` is the class that represents the instruction/Value itself, or the pseudo instruction.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:833-837
+        if (Instruction *I = dyn_cast<Instruction>(V)) {
+           Last->State.MainOp = I;
+           Last->State.AltOp = I;
+           break;
+        }
----------------
Why do you need this?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:853
 
-  TreeEntry *getTreeEntry(Value *V) {
-    auto I = ScalarToTreeEntry.find(V);
-    if (I != ScalarToTreeEntry.end())
-      return &VectorizableTree[I->second];
+  TreeEntry *getTreeEntry(Instruction *I) {
+    if (!I)
----------------
Again, you need all this code because you did not implemented what we discussed. Try to use the `InstructionOrPseudoInstruction` like class to represent values/instructions and pseudo-instructions. It should be much easier to implement and a lot of changes will just go away.


https://reviews.llvm.org/D28907