[llvm] r277980 - [PM] Invalidate CallGraphAnalysis because it holds AssertingVH

[Chandler Carruth via llvm-commits]

On Sun, Aug 7, 2016 at 11:56 PM Sean Silva <chisophugis at gmail.com> wrote:

> On Sun, Aug 7, 2016 at 11:40 PM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
>> On Sun, Aug 7, 2016 at 10:45 PM Sean Silva via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: silvas
>>> Date: Mon Aug  8 00:38:01 2016
>>> New Revision: 277980
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=277980&view=rev
>>> Log:
>>> [PM] Invalidate CallGraphAnalysis because it holds AssertingVH
>>>
>>> This is essentially PR28400. The fix here is similar to that implemented
>>> in r274656.
>>>
>>
>> This patch, without a test case, is very hard to understand. It doesn't
>> seem to make sense in isolation.
>>
>> Maybe this makes more sense in combination with the large patch around
>> invalidation you mailed out? If so, I don't think it should just be
>> committed.
>>
>
> No, this is unrelated to that patch. This is related to the thread "Should
> analyses be able to hold AssertingVH to IR? (related to PR28400)" linked
> from r274656 which was mentioned in the commit message.
>
>
>
>
>>
>> If this makes sense outside that patch, I would expect a test case? Maybe
>> discuss this before committing it?
>>
>>
>
> I described this workaround in that thread and there didn't seem to be any
> objections to doing that for now.
>

I don't think that really counts as code review, nor does it really help
other readers of commits who may not have read the thread you are
mentioning. Assuming everyone has read that thread doesn't seem reasonable
IMO.

Perhaps revert these workarounds and send out a patch for review with them,
and include a succinct summary of what the plan is here?

Again, I'm not trying to say this is necessarily the wrong approach, I just
think you need to go through the right steps here to make sure folks are
aware and understand the implications.


>
> -- Sean Silva
>
>
>
>>
>>> Modified:
>>>     llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
>>>
>>> Modified: llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp?rev=277980&r1=277979&r2=277980&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/IPO/FunctionAttrs.cpp Mon Aug  8 00:38:01
>>> 2016
>>> @@ -1271,6 +1271,11 @@ ReversePostOrderFunctionAttrsPass::run(M
>>>    auto &CG = AM.getResult<CallGraphAnalysis>(M);
>>>
>>>    bool Changed = deduceFunctionAttributeInRPO(M, CG);
>>> +
>>> +  // CallGraphAnalysis holds AssertingVH and must be invalidated
>>> eagerly so
>>> +  // that other passes don't delete stuff from under it.
>>> +  AM.invalidate<CallGraphAnalysis>(M);
>>> +
>>>    if (!Changed)
>>>      return PreservedAnalyses::all();
>>>    PreservedAnalyses PA;
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160808/912a7b68/attachment.html>