[PATCH] D22918: [Loop Vectorizer] Support predication of div/rem

[Michael Kuperstein via llvm-commits]

mkuper added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3327
@@ -3321,1 +3326,3 @@
+      if (Predicate)
+        Cost += TTI.getCFInstrCost(Instruction::PHI);
       Cost += TTI.getVectorInstrCost(Instruction::InsertElement, Ty, I);
----------------
I'm not entirely sure what "the cost of a phi" means, especially without a type.

Also, I don't believe any in-tree target actually assigns a non-zero to PHIs right now. So if you actually want a meaningful cost here for, say, x86, you may want to look into the cost model as well...

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3825
@@ -3781,3 +3824,3 @@
 
-  // Predicate any stores.
-  for (auto KV : PredicatedStores) {
+  // Predicate any instructions.
+  for (auto KV : PredicatedInstructions) {
----------------
anemet wrote:
> Only a drive-by comment, please don't make vectorizeLoop any more unreadable than it already is.  Please consider prequel to this patch that moves store-predication into its own function.
...that need predication?
(Before, we really would predicate *any* store)

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3825-3834
@@ -3781,10 +3824,12 @@
 
-  // Predicate any stores.
-  for (auto KV : PredicatedStores) {
+  // Predicate any instructions.
+  for (auto KV : PredicatedInstructions) {
     BasicBlock::iterator I(KV.first);
-    auto *BB = SplitBlock(I->getParent(), &*std::next(I), DT, LI);
+    BasicBlock *Head = I->getParent();
+    auto *BB = SplitBlock(Head, &*std::next(I), DT, LI);
     auto *T = SplitBlockAndInsertIfThen(KV.second, &*I, /*Unreachable=*/false,
                                         /*BranchWeights=*/nullptr, DT, LI);
     I->moveBefore(T);
-    I->getParent()->setName("pred.store.if");
-    BB->setName("pred.store.continue");
+    // Try to move any extract-element we may have created for the predicated
+    // instruction into the Then block.
+    for (unsigned In = 0; In < I->getNumOperands(); ++In) {
----------------
mkuper wrote:
> anemet wrote:
> > Only a drive-by comment, please don't make vectorizeLoop any more unreadable than it already is.  Please consider prequel to this patch that moves store-predication into its own function.
> ...that need predication?
> (Before, we really would predicate *any* store)
+1

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3833
@@ -3788,3 +3832,3 @@
     I->moveBefore(T);
-    I->getParent()->setName("pred.store.if");
-    BB->setName("pred.store.continue");
+    // Try to move any extract-element we may have created for the predicated
+    // instruction into the Then block.
----------------
We don't run anything that will sink this later in the pipeline?
(To be honest, even if we do, I'm not sure whether we should do the cleanup here or rely on a later pass, but I'm curious)

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3835
@@ +3834,3 @@
+    // instruction into the Then block.
+    for (unsigned In = 0; In < I->getNumOperands(); ++In) {
+      ExtractElementInst *OpInst =
----------------
Any reason not to use a range for over the operands?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3841
@@ +3840,3 @@
+      bool CanSinkToUse = true;
+      for (User *U : OpInst->users()) {
+        if (U != &*I) {
----------------
Do you know if we have an isOnlyUserOf helper?
I know we have one for SDNode... but, I couldn't find one in IR.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3859
@@ +3858,3 @@
+      auto Users = I->users();
+      if (std::distance(Users.begin(), Users.end()) == 1 &&
+          isa<InsertElementInst>(*I->users().begin())) {
----------------
I->hasOneUse()? Or do you care specifically about the users() list? In any case, no need to compute std::distance.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3860
@@ +3859,3 @@
+      if (std::distance(Users.begin(), Users.end()) == 1 &&
+          isa<InsertElementInst>(*I->users().begin())) {
+        // If the predicated instruction is feeding an insert-element, move it
----------------
I->user_begin()?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4251
@@ +4250,3 @@
+    case Instruction::SRem:
+    case Instruction::FRem:
+      // Scalarize if block needs predication, otherwise fallthrough.
----------------
FRem looks out of place. Did you mean URem?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4254
@@ +4253,3 @@
+      if (Legal->blockNeedsPredication(I.getParent())) {
+        scalarizeInstruction(&I, true);
+        continue;
----------------
To expand on what Eli said, if we have division by a non-zero constant, then:
1) It may be efficiently lowered.
2) Since the constant is non-zero, it doesn't need predication.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4264
@@ -4167,5 +4263,3 @@
     case Instruction::FDiv:
     case Instruction::URem:
     case Instruction::Shl:
----------------
And, correspondingly, this should probably be FRem.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5070
@@ -4975,3 +5069,3 @@
 
     // We don't predicate stores at the moment.
     if (I.mayWriteToMemory()) {
----------------
As long as you're touching this - remove this comment?



https://reviews.llvm.org/D22918