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...<div>
<br></div><div>I've attached updated patches with all of Duncan's comments addressed (I think).<br><br></div><div><div class="gmail_quote">On Thu, Mar 22, 2012 at 9:30 AM, Duncan Sands <span dir="ltr"><<a href="mailto:baldrick@free.fr">baldrick@free.fr</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Chandler,<br>
<br>
> --- a/lib/Transforms/Utils/CloneFunction.cpp<br>
> +++ b/lib/Transforms/Utils/CloneFunction.cpp<br>
> @@ -25,6 +25,7 @@<br>
> #include "llvm/Support/CFG.h"<br>
> #include "llvm/Transforms/Utils/ValueMapper.h"<br>
> #include "llvm/Analysis/ConstantFolding.h"<br>
> +#include "llvm/Analysis/InstructionSimplify.h"<br>
> #include "llvm/Analysis/DebugInfo.h"<br>
> #include "llvm/ADT/SmallVector.h"<br>
> #include <map><br>
> @@ -218,11 +219,6 @@ namespace {<br>
> /// anything that it can reach.<br>
> void CloneBlock(const BasicBlock *BB,<br>
> std::vector<const BasicBlock*> &ToClone);<br>
> -<br>
> - public:<br>
> - /// ConstantFoldMappedInstruction - Constant fold the specified instruction,<br>
> - /// mapping its operands through VMap if they are available.<br>
> - Constant *ConstantFoldMappedInstruction(const Instruction *I);<br>
> };<br>
> }<br>
><br>
> @@ -262,19 +258,27 @@ void PruningFunctionCloner::CloneBlock(const BasicBlock *BB,<br>
> // loop doesn't include the terminator.<br>
> for (BasicBlock::const_iterator II = BB->begin(), IE = --BB->end();<br>
> II != IE; ++II) {<br>
> - // If this instruction constant folds, don't bother cloning the instruction,<br>
> - // instead, just add the constant to the value map.<br>
> - if (Constant *C = ConstantFoldMappedInstruction(II)) {<br>
> - VMap[II] = C;<br>
> - continue;<br>
> + Instruction *NewInst = II->clone();<br>
> +<br>
> + // Eagerly remap operands to the newly cloned instruction, except for PHI<br>
> + // nodes for which we defer processing until we update the CFG.<br>
> + if (!isa<PHINode>(NewInst)) {<br>
<br>
Do you also need to skip terminators here?<br></blockquote><div><br></div><div>We do. The loop backs up 'end' by one.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + RemapInstruction(NewInst, VMap,<br>
> + ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);<br>
> +<br>
> + // If this instruction folds don't bother cloning it, just add the<br>
<br>
You already cloned it, maybe the comment should say: don't bother inserting it<br></blockquote><div><br></div><div>Clarified.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, since you aren't remapping phi nodes at this point, what happens if you<br>
call instsimplify on an instruction with a phi node for an operand? Might<br>
instsimplify come to the wrong conclusion, or fail to perform a simplification<br>
that it might otherwise have done?<br></blockquote><div><br></div><div>I believe that it cannot come to an incorrect conclusion, however it might fail to perform some simplification.</div><div><br></div><div>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.</div>
<div><br></div><div>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> @@ -418,25 +398,19 @@ void llvm::CloneAndPruneFunctionInto(Function *NewFunc, const Function *OldFunc,<br>
><br>
> // Add the new block to the new function.<br>
> NewFunc->getBasicBlockList().push_back(NewBB);<br>
> -<br>
> - // Loop over all of the instructions in the block, fixing up operand<br>
> - // references as we go. This uses VMap to do all the hard work.<br>
> - //<br>
> - BasicBlock::iterator I = NewBB->begin();<br>
><br>
> // Handle PHI nodes specially, as we have to remove references to dead<br>
> // blocks.<br>
> - if (PHINode *PN = dyn_cast<PHINode>(I)) {<br>
> - // Skip over all PHI nodes, remembering them for later.<br>
> - BasicBlock::const_iterator OldI = BI->begin();<br>
> - for (; (PN = dyn_cast<PHINode>(I)); ++I, ++OldI)<br>
> - PHIToResolve.push_back(cast<PHINode>(OldI));<br>
> - }<br>
> -<br>
> - // Otherwise, remap the rest of the instructions normally.<br>
> - for (; I != NewBB->end(); ++I)<br>
> - RemapInstruction(I, VMap,<br>
> - ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);<br>
> + for (BasicBlock::const_iterator I = BI->begin(), E = BI->end(); I != E; ++I)<br>
> + if (const PHINode *PN = dyn_cast<PHINode>(I))<br>
> + PHIToResolve.push_back(PN);<br>
> + else<br>
> + break;<br>
<br>
Is there any point in pushing all these phi nodes into a vector, just so they<br>
can be read out again below? Can't the phi resolution code below just directly<br>
iterate over basic blocks and the phi nodes in them?<br></blockquote><div><br></div><div>Possibly, but I didn't want to change that aspect of the code in this patch.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + // Finally, remap the terminator instructions, as those can't be remapped<br>
> + // until all BBs are mapped.<br>
> + RemapInstruction(NewBB->getTerminator(), VMap,<br>
> + ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);<br>
<br>
Didn't you remap these earlier already?<br></blockquote><div><br></div><div>No, see above. We do actually skip terminators.</div></div></div>