[llvm-commits] [llvm] r72364 - in /llvm/trunk: include/llvm/Analysis/ScalarEvolutionExpander.h lib/Analysis/ScalarEvolutionExpander.cpp lib/Transforms/Scalar/IndVarSimplify.cpp test/Transforms/IndVarSimplify/2009-05-24-useafterfree.ll

[Dan Gohman]

Hi Edwin,

Thanks for fixing this.  I have some comments below.

On May 24, 2009, at 7:23 AM, Torok Edwin wrote:

> Author: edwin
> Date: Sun May 24 09:23:16 2009
> New Revision: 72364
>
> URL: http://llvm.org/viewvc/llvm-project?rev=72364&view=rev
> Log:
> The rewriter may hold references to instructions that are deleted  
> because they are trivially dead.
> Fix by clearing the rewriter cache before deleting the trivially dead
> instructions.
> Also make InsertedExpressions use an AssertingVH to catch these
> bugs easier.
>
> Added:
>    llvm/trunk/test/Transforms/IndVarSimplify/2009-05-24- 
> useafterfree.ll
> Modified:
>    llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h
>    llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
>    llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h?rev=72364&r1=72363&r2=72364&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h  
> (original)
> +++ llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h Sun  
> May 24 09:23:16 2009
> @@ -28,7 +28,7 @@
>   /// memory.
>   struct SCEVExpander : public SCEVVisitor<SCEVExpander, Value*> {
>     ScalarEvolution &SE;
> -    std::map<SCEVHandle, Value*> InsertedExpressions;
> +    std::map<SCEVHandle, AssertingVH<Value> > InsertedExpressions;
>     std::set<Value*> InsertedValues;
>
>     BasicBlock::iterator InsertPt;
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp?rev=72364&r1=72363&r2=72364&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolutionExpander.cpp Sun May 24  
> 09:23:16 2009
> @@ -526,7 +526,7 @@
>
> Value *SCEVExpander::expand(const SCEV *S) {
>   // Check to see if we already expanded this.
> -  std::map<SCEVHandle, Value*>::iterator I =  
> InsertedExpressions.find(S);
> +  std::map<SCEVHandle, AssertingVH<Value> >::iterator I =  
> InsertedExpressions.find(S);

This line exceeds 80 columns.

>
>   if (I != InsertedExpressions.end())
>     return I->second;
>
>
> Modified: llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp?rev=72364&r1=72363&r2=72364&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp Sun May 24  
> 09:23:16 2009
> @@ -298,6 +298,7 @@
>         // in the loop, so we don't need an LCSSA phi node anymore.
>         if (NumPreds == 1) {
>           PN->replaceAllUsesWith(ExitVal);
> +          Rewriter.clear();

>
>           RecursivelyDeleteTriviallyDeadInstructions(PN);

For this case, the only time this will actually delete anything
other than PN is when PN itself was dead before indvars
came along.  In this case, I think it would be better to have
a check at the top of the loop for PN->use_empty(), so that
indvars doesn't bother expanding a replacement value
for a dead use.  That would save work, and it would avoid
the need to completely clear the Rewriter's cache.

>
>           break;
>         }
> @@ -418,6 +419,7 @@
>   // Reorder instructions to avoid use-before-def conditions.
>   FixUsesBeforeDefs(L, Rewriter);
>
> +  Rewriter.clear();
>   // For completeness, inform IVUsers of the IV use in the newly- 
> created
>   // loop exit test instruction.
>   if (NewICmp)


At the end of RewriteIVExpressions, there's a call to
RecursivelyDeleteTriviallyDeadInstructions which looks like
it also could trigger the AssertingVH.  In that case and in the
one here, the AssertingVH is slightly more strict than necessary,
because at either point, indvars is done using the Rewriter to
expand new expressions.  Unfortunately, the Rewriter can't
currently be cleared by RewriteIVExpressions because the
other functions still need to call isInsertedExpression.

I agree that using AssertingVH seems desirable, though I
don't yet know how best to arrange this code for it.

Dan