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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev 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:
> 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 
Better to do it in a follow-up too. :)


================
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:
> 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
Yes, it's better to do it in a follow-up.


https://reviews.llvm.org/D51581