[llvm-commits] PATCH: Followup to r152556: Use more powerful instsimplify when actually performing the inline of a function

[Duncan Sands]

Hi Chandler,

> --- a/lib/Transforms/Utils/CloneFunction.cpp
> +++ b/lib/Transforms/Utils/CloneFunction.cpp
> @@ -25,6 +25,7 @@
>  #include "llvm/Support/CFG.h"
>  #include "llvm/Transforms/Utils/ValueMapper.h"
>  #include "llvm/Analysis/ConstantFolding.h"
> +#include "llvm/Analysis/InstructionSimplify.h"
>  #include "llvm/Analysis/DebugInfo.h"
>  #include "llvm/ADT/SmallVector.h"
>  #include <map>
> @@ -218,11 +219,6 @@ namespace {
>      /// anything that it can reach.
>      void CloneBlock(const BasicBlock *BB,
>                      std::vector<const BasicBlock*> &ToClone);
> -
> -  public:
> -    /// ConstantFoldMappedInstruction - Constant fold the specified instruction,
> -    /// mapping its operands through VMap if they are available.
> -    Constant *ConstantFoldMappedInstruction(const Instruction *I);
>    };
>  }
>
> @@ -262,19 +258,27 @@ void PruningFunctionCloner::CloneBlock(const BasicBlock *BB,
>    // loop doesn't include the terminator.
>    for (BasicBlock::const_iterator II = BB->begin(), IE = --BB->end();
>         II != IE; ++II) {
> -    // If this instruction constant folds, don't bother cloning the instruction,
> -    // instead, just add the constant to the value map.
> -    if (Constant *C = ConstantFoldMappedInstruction(II)) {
> -      VMap[II] = C;
> -      continue;
> +    Instruction *NewInst = II->clone();
> +
> +    // Eagerly remap operands to the newly cloned instruction, except for PHI
> +    // nodes for which we defer processing until we update the CFG.
> +    if (!isa<PHINode>(NewInst)) {

Do you also need to skip terminators here?

> +      RemapInstruction(NewInst, VMap,
> +                       ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);
> +
> +      // If this instruction folds don't bother cloning it, just add the

You already cloned it, maybe the comment should say: don't bother inserting it

Also, since you aren't remapping phi nodes at this point, what happens if you
call instsimplify on an instruction with a phi node for an operand?  Might
instsimplify come to the wrong conclusion, or fail to perform a simplification
that it might otherwise have done?

> @@ -418,25 +398,19 @@ void llvm::CloneAndPruneFunctionInto(Function *NewFunc, const Function *OldFunc,
>
>      // Add the new block to the new function.
>      NewFunc->getBasicBlockList().push_back(NewBB);
> -
> -    // Loop over all of the instructions in the block, fixing up operand
> -    // references as we go.  This uses VMap to do all the hard work.
> -    //
> -    BasicBlock::iterator I = NewBB->begin();
>
>      // Handle PHI nodes specially, as we have to remove references to dead
>      // blocks.
> -    if (PHINode *PN = dyn_cast<PHINode>(I)) {
> -      // Skip over all PHI nodes, remembering them for later.
> -      BasicBlock::const_iterator OldI = BI->begin();
> -      for (; (PN = dyn_cast<PHINode>(I)); ++I, ++OldI)
> -        PHIToResolve.push_back(cast<PHINode>(OldI));
> -    }
> -
> -    // Otherwise, remap the rest of the instructions normally.
> -    for (; I != NewBB->end(); ++I)
> -      RemapInstruction(I, VMap,
> -                       ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);
> +    for (BasicBlock::const_iterator I = BI->begin(), E = BI->end(); I != E; ++I)
> +      if (const PHINode *PN = dyn_cast<PHINode>(I))
> +        PHIToResolve.push_back(PN);
> +      else
> +        break;

Is there any point in pushing all these phi nodes into a vector, just so they
can be read out again below?  Can't the phi resolution code below just directly
iterate over basic blocks and the phi nodes in them?

> +
> +    // Finally, remap the terminator instructions, as those can't be remapped
> +    // until all BBs are mapped.
> +    RemapInstruction(NewBB->getTerminator(), VMap,
> +                     ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);

Didn't you remap these earlier already?

>    }
>
>    // Defer PHI resolution until rest of function is resolved, PHI resolution

Ciao, Duncan.