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 ↗

(On Diff #88949)

Why?

1465 ↗

(On Diff #88949)

This change looks fine.

1488 ↗

(On Diff #88949)

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 ↗

(On Diff #88949)

I can remove this if you want.

1488 ↗

(On Diff #88949)

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/trunk/lib/Transforms/Utils/SimplifyCFG.cpp

if (!all_of(Insts, [&PNUse,&Succ](const Instruction *I) -> bool {
           return (PNUse &&
                   PNUse->getParent() == Succ &&
                   PNUse->getIncomingValueForBlock(I->getParent()) == I) ||
                  U->getParent() == I->getParent();
         }))
       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;
+  if (isa<StoreInst>(I0) && any_of(Insts, [](const Instruction *I) {
+  if (isa<LoadInst>(I0) && any_of(Insts, [](const Instruction *I) {
 
+  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;
+
     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;