[PATCH] D51581: [IndVars] Strengthen restricton in rewriteLoopExitValues

[Hongbin Zheng via Phabricator via llvm-commits]

etherzhhb added inline comments.


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:605-615
           for (auto *IB : Inst->users()) {
             Instruction *UseInstr = cast<Instruction>(IB);
             unsigned Opc = UseInstr->getOpcode();
-            if (L->contains(UseInstr)) {
-              if (Opc == Instruction::Call)
-                HasHardInternalUses = true;
-            } else {
-              if (Opc == Instruction::PHI) {
-                // Do not count the Phi as a use. LCSSA may have inserted
-                // plenty of trivial ones.
-                for (auto *PB : UseInstr->users()) {
-                  unsigned PhiOpc = cast<Instruction>(PB)->getOpcode();
-                  if (PhiOpc != Instruction::Call &&
-                      PhiOpc != Instruction::Ret) {
-                    HasSoftExternalUses = true;
-                    break;
-                  }
-                }
-                continue;
-              }
-              if (Opc != Instruction::Call && Opc != Instruction::Ret) {
-                HasSoftExternalUses = true;
-                break;
-              }
+            if (L->contains(UseInstr) && Opc == Instruction::Call) {
+              HasHardInternalUses = true;
+              break;
             }
----------------
etherzhhb wrote:
> we can further write:
> 
> ```
> // Skip if  Inst is used inside the loop in a way which can not be
> //    optimized away.
> if (llvm::any_of(Inst->users(), [L](auto *U) {
>   return isa<CallInst>(UseInstr) && L->contains(UseInstr);
> }))
>   continue;
> ```
> 
> which is easier to read
I forgot there is a follow up patch to rewrite this :)

You could decide if you want to rewrite the loop or leave it to the next patch 


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:607-608
             Instruction *UseInstr = cast<Instruction>(IB);
             unsigned Opc = UseInstr->getOpcode();
-            if (L->contains(UseInstr)) {
-              if (Opc == Instruction::Call)
-                HasHardInternalUses = true;
-            } else {
-              if (Opc == Instruction::PHI) {
-                // Do not count the Phi as a use. LCSSA may have inserted
-                // plenty of trivial ones.
-                for (auto *PB : UseInstr->users()) {
-                  unsigned PhiOpc = cast<Instruction>(PB)->getOpcode();
-                  if (PhiOpc != Instruction::Call &&
-                      PhiOpc != Instruction::Ret) {
-                    HasSoftExternalUses = true;
-                    break;
-                  }
-                }
-                continue;
-              }
-              if (Opc != Instruction::Call && Opc != Instruction::Ret) {
-                HasSoftExternalUses = true;
-                break;
-              }
+            if (L->contains(UseInstr) && Opc == Instruction::Call) {
+              HasHardInternalUses = true;
----------------
etherzhhb wrote:
> since opc is used only once now, maybe we could write:
> 
> ```
> if (isa<CallInst>(UseInstr) && L->contains(UseInstr)) {
> ```
> 
> Also, how about invoke?
I think the invoke is addressed in the follow up patch


https://reviews.llvm.org/D51581