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


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:293
+  if (!Res.NeedAnalysis && !Res.HasAltOpcodes)
+    return {Opcode, VL[Res.OpcodePos], /*IsAltShuffle=*/false};
+  auto *OpInst = cast<Instruction>(VL[Res.OpcodePos]);
----------------
For clarity, use the InstructionsState constructor not an initializer.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:305
+        (!Res.HasAltOpcodes && InstOpcode != Opcode)) {
+      return {0, OpInst,  /*IsAltShuffle=*/false};
     }
----------------
For clarity, use the InstructionsState constructor not an initializer.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:308
   }
-  return Opcode;
+  return {Opcode, OpInst, Res.HasAltOpcodes};
+}
----------------
For clarity, use the InstructionsState constructor not an initializer.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3265
+    // No need to handle users of gathered values.
+    if (Entry->NeedToGather)
+      continue;
----------------
Can this (and the assert below) be pulled out of the inner loop as an NFC commit now? It looks loop invariant.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3480
+void BoUpSLP::BlockScheduling::cancelScheduling(ArrayRef<Value *> VL,
+                                                Value *OpValue) {
+  if (isa<PHINode>(OpValue))
----------------
You might be able to update cancelScheduling to use this extra arg as a NFC, and add VL0 in the calls.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3564
         ScheduleStart = I;
+        if (chooseKey(OpValue, I) != I) CheckSheduleForI(I);
         DEBUG(dbgs() << "SLP:  extend schedule region start to " << *I << "\n");
----------------
(style) Put the  CheckSheduleForI(I); on a newline


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:3575
         ScheduleEnd = I->getNextNode();
+        if (chooseKey(OpValue, I) != I) CheckSheduleForI(I);
         assert(ScheduleEnd && "tried to vectorize a TerminatorInst?");
----------------
(style) Put the  CheckSheduleForI(I); on a newline


https://reviews.llvm.org/D28907