[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 22:05, Török Edwin wrote:

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

Actually I think the Rewriter.clear() could be moved before those last 2
functions that use the Rewriter.
FixUsesBeforeDefs and SinkUnusedInvariants only need isInsertedInstruction.
 That one is uses the InsertedValues set which doesn't use AssertingVH,
it is also not cleared by Rewriter.clear().

In fact only RewriteIVExpressions needs isInsertedExpression, so I think
it would be safe to clear the Rewriter map right after calling
RewriteIVExpressions.

What do  you think? Or should I just remove the AssertingVH until we
find a better solution?

Best regards,
--Edwin