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

Some minor observations. It'd be great it we could reduce the size of this patch some how....



================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:208
+static unsigned tryToRepresentAsInstArg(unsigned Opcode, Instruction *I) {
+  assert(!sameOpcodeOrAlt(Opcode, getAltOpcode(Opcode), I->getOpcode()));
+  if (Opcode != Instruction::PHI &&
----------------
Always add an assert message.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:682
   /// Maps a specific scalar to its tree entry.
-  SmallDenseMap<Value*, int> ScalarToTreeEntry;
+  SmallDenseMap<Value *, int> ScalarToTreeEntry;
+
----------------
clang-format NFC pre-commit


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:788
       clearDependencies();
+      this->OpValue = OpValue;
     }
----------------
Can you just change the name of either the argument or member to avoid the this-> ?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1263
+    const unsigned AltOpcode = getAltOpcode(Opcode);
     for (int Lane = 0, LE = Entry->Scalars.size(); Lane != LE; ++Lane) {
       Value *Scalar = Entry->Scalars[Lane];
----------------
for (auto *Scalar : Entry->Scalars)

As a NFC pre-commit if possible


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1470
 
-  switch (Opcode) {
+  switch (S.IsAltShuffle ? Instruction::ShuffleVector : S.Opcode) {
     case Instruction::PHI: {
----------------
Please don't embed this in the switch()


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1682
         // Prepare the operand vector.
-        for (Value *j : VL)
-          Operands.push_back(cast<Instruction>(j)->getOperand(i));
+        for (Value *V : VL) {
+          auto *I = cast<Instruction>(V);
----------------
Do the j->V rename as a NFC pre-commit


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1860
+          if (!sameOpcodeOrAlt(S.Opcode, AltOpcode, I->getOpcode())) {
+            assert(Instruction::isBinaryOp(S.Opcode));
+            Operand = isOdd(i)
----------------
Always add an assert message.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:1972
+  switch (E->State.IsAltShuffle ? Instruction::ShuffleVector
+                                : E->State.Opcode) {
     case Instruction::PHI: {
----------------
Please don't embed this in the switch()


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2433
+static bool shouldReorderOperands(
+    int Idx, unsigned Opcode, Instruction &I, ArrayRef<Value *> Left,
+    ArrayRef<Value *> Right, bool AllSameOpcodeLeft, bool AllSameOpcodeRight,
----------------
The i ->Idx change can be pulled out of a NFC pre-commit.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:2739
+  switch (E->State.IsAltShuffle ?
+          Instruction::ShuffleVector : E->State.Opcode) {
     case Instruction::PHI: {
----------------
Please don't embed this in the switch()


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3113
+      InstructionsState S = getSameOpcode(EvenScalars);
+      assert(!S.IsAltShuffle);
+      propagateIRFlagsWithOp(V0, EvenScalars, S.OpValue);
----------------
Always add an assert message.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3117
+      S = getSameOpcode(OddScalars);
+      assert(!S.IsAltShuffle);
+      propagateIRFlagsWithOp(V1, OddScalars, S.OpValue);
----------------
Always add an assert message.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3267
+    const unsigned AltOpcode = getAltOpcode(Opcode);
     for (int Lane = 0, LE = Entry->Scalars.size(); Lane != LE; ++Lane) {
       Value *Scalar = Entry->Scalars[Lane];
----------------
for (auto *Scalar : Entry->Scalars)

Possibly as a NFC pre-commit.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3537
+    if (chooseKey(OpValue, I) != I)
+      (void)CheckSheduleForI(I);
     assert(ScheduleEnd && "tried to vectorize a TerminatorInst?");
----------------
Do we need the (void)?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3560
+        if (chooseKey(OpValue, I) != I)
+          (void)CheckSheduleForI(I);
         DEBUG(dbgs() << "SLP:  extend schedule region start to " << *I << "\n");
----------------
Do we need the (void)?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3572
+        if (chooseKey(OpValue, I) != I)
+          (void)CheckSheduleForI(I);
         assert(ScheduleEnd && "tried to vectorize a TerminatorInst?");
----------------
Do we need the (void)?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3661
+                if (!DestBundle->hasValidDependencies())
+                  WorkList.push_back(DestBundle);
               }
----------------
Remove these brackets - they can be done as a NFC commit right away and not affect this patch. 


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4872
+        InstructionsState S = getSameOpcode(ReductionOps);
+        assert(!S.IsAltShuffle);
+        propagateIRFlagsWithOp(VectorizedTree, ReductionOps, S.OpValue);
----------------
Always add an assert message.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4888
+        InstructionsState S = getSameOpcode(ReductionOps);
+        assert(!S.IsAltShuffle);
+        propagateIRFlagsWithOp(VectorizedTree, ReductionOps, S.OpValue);
----------------
Always add an assert message.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4900
+          InstructionsState S = getSameOpcode(I);
+          assert(!S.IsAltShuffle);
+          propagateIRFlagsWithOp(VectorizedTree, I, S.OpValue);
----------------
Always add an assert message.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4968
+      InstructionsState S = getSameOpcode(RedOps);
+      assert(!S.IsAltShuffle);
+      propagateIRFlagsWithOp(TmpVec, RedOps, S.OpValue);
----------------
Always add an assert message.


https://reviews.llvm.org/D28907