[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

[Török Edwin]

On 2009-05-24 21:49, Dan Gohman wrote:
> Hi Edwin,
>
> Thanks for fixing this.  I have some comments below.
>
>   
>> 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.
>   

Will fix.

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

Thanks, will try that.

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

Maybe instead of AssertingVH we could use CallbackVH, and have it assert
only if you
want to dereference a deleted Value*. Does that sound reasonable?

Best regards,
--Edwin