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

[Chandler Carruth]

FYI, these patches are now pretty well tested (nightly test suite) and the
only regression is fixed by the patch to instsimplify I just mailed. I'm
planning to commit as soon as those go in unless there are new issues
raised soon...

I've attached updated patches with all of Duncan's comments addressed (I
think).

On Thu, Mar 22, 2012 at 9:30 AM, Duncan Sands <baldrick at free.fr> wrote:

> 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?
>

We do. The loop backs up 'end' by one.


> > +      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
>

Clarified.


> 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?
>

I believe that it cannot come to an incorrect conclusion, however it might
fail to perform some simplification.

If we end up looking into the old function via a phi-node's un-mapped
operand, we should draw conclusions which are correct for the old function.
Since the old function is being inlined, all conclusions which hold about
the old function's values must also hold about the new function's values
conservatively. The only edge case I wasn't handling is that if we simplify
to an instruction in the old function, we need to map it back to the new
function. I've fixed that.

However, the new function may introduce new simplifications not possible in
the old function. We will miss these when simplifying an instruction whose
operand is a phi node, which is why when we do the simplification of phi
nodes in a deferred pass, we also do *recursive* simplification. That
should clean up all of the missed simplifications here.


> > @@ -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?
>

Possibly, but I didn't want to change that aspect of the code in this patch.


>
> > +
> > +    // 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?
>

No, see above. We do actually skip terminators.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120323/5ca871b7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inline-instsimplify.patch
Type: application/octet-stream
Size: 8384 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120323/5ca871b7/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: inliner-phi-simplify.diff
Type: application/octet-stream
Size: 2918 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120323/5ca871b7/attachment-0001.obj>