Refactor SimplifyCFG:canSinkInstructions [NFC]

[hiraditya]

Ping

[jmolloy]

Hi,

Can you please explain what you are doing and why? Is there a functional change here? or just moving code around?

I can see a functional change

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1454

Why?

1465

This change looks fine.

1488

Your new code is ignoring the operand index, so is more conservative than the old code.

[hiraditya]

This patch is only to move the code around to avoid + avoid redundant computations.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1454

I can remove this if you want.

1488

This code will only execute when IO is a store instruction. And stores have 2 operands, so that implies OI=1 is not needed when we hoist this code out of the loop. It appears OI==1 check was there because for stores second operand is the pointer operand, and for stores first operand (OI==0) is the pointer operand.

[hiraditya]

pimg

[jmolloy]

LGTM

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

 
     // Everything must have only one use too, apart from stores which
     // have no uses.
     if (!isa<StoreInst>(I) && !I->hasOneUse())
       return false;
   }
 
   const Instruction *I0 = Insts.front();
-  for (auto *I : Insts)
+  for (Instruction *I : Insts)
     if (!I->isSameOperationAs(I0))
       return false;
 
   // All instructions in Insts are known to be the same opcode. If they aren't
   // stores, check the only user of each is a PHI or in the same block as the
   // instruction, because if a user is in the same block as an instruction
   // we're contemplating sinking, it must already be determined to be sinkable.
   if (!isa<StoreInst>(I0)) {
     auto *PNUse = dyn_cast<PHINode>(*I0->user_begin());
     auto *Succ = I0->getParent()->getTerminator()->getSuccessor(0);
-    if (!all_of(Insts, [&PNUse,&Succ](const Instruction *I) -> bool {
+    if (!PNUse || PNUse->getParent() != Succ)
+      return false;
+    if (!all_of(Insts, [&PNUse](const Instruction *I) -> bool {
           auto *U = cast<Instruction>(*I->user_begin());
-          return (PNUse &&
-                  PNUse->getParent() == Succ &&
-                  PNUse->getIncomingValueForBlock(I->getParent()) == I) ||
-                 U->getParent() == I->getParent();
+          return (PNUse->getIncomingValueForBlock(I->getParent()) == I) ||
+                  U->getParent() == I->getParent();
         }))
       return false;
   }
 
+  // Because SROA can't handle speculating stores of selects, try not
+  // to sink loads or stores of allocas when we'd have to create a PHI for
+  // the address operand. Also, because it is likely that loads or stores
+  // of allocas will disappear when Mem2Reg/SROA is run, don't sink them.
+  // This can cause code churn which can have unintended consequences down
+  // the line - see https://llvm.org/bugs/show_bug.cgi?id=30244.
+  // FIXME: This is a workaround for a deficiency in SROA - see
+  // https://llvm.org/bugs/show_bug.cgi?id=30188
+  if (isa<StoreInst>(I0) &&
+      any_of(Insts, [](const Instruction *I) {
+        return isa<AllocaInst>(I->getOperand(1));
+      }))
+    return false;
+  if (isa<LoadInst>(I0) && any_of(Insts, [](const Instruction *I) {
+        return isa<AllocaInst>(I->getOperand(0));
+      }))
+    return false;
+
   for (unsigned OI = 0, OE = I0->getNumOperands(); OI != OE; ++OI) {
     if (I0->getOperand(OI)->getType()->isTokenTy())
       // Don't touch any operand of token type.
       return false;
 
-    // Because SROA can't handle speculating stores of selects, try not
-    // to sink loads or stores of allocas when we'd have to create a PHI for
-    // the address operand. Also, because it is likely that loads or stores
-    // of allocas will disappear when Mem2Reg/SROA is run, don't sink them.
-    // This can cause code churn which can have unintended consequences down
-    // the line - see https://llvm.org/bugs/show_bug.cgi?id=30244.
-    // FIXME: This is a workaround for a deficiency in SROA - see
-    // https://llvm.org/bugs/show_bug.cgi?id=30188
-    if (OI == 1 && isa<StoreInst>(I0) &&
-        any_of(Insts, [](const Instruction *I) {
-          return isa<AllocaInst>(I->getOperand(1));
-        }))
-      return false;
-    if (OI == 0 && isa<LoadInst>(I0) && any_of(Insts, [](const Instruction *I) {
-          return isa<AllocaInst>(I->getOperand(0));
-        }))
-      return false;
-
     auto SameAsI0 = [&I0, OI](const Instruction *I) {
       assert(I->getNumOperands() == I0->getNumOperands());
       return I->getOperand(OI) == I0->getOperand(OI);
     };
     if (!all_of(Insts, SameAsI0)) {
       if (!canReplaceOperandWithVariable(I0, OI))
         // We can't create a PHI from this GEP.
         return false;