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

[Sean Silva via llvm-commits]

On Mon, Aug 8, 2016 at 12:21 AM, Chandler Carruth <chandlerc at google.com>
wrote:

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

I think it is totally reasonable to expect readers to consider the context
of any linked commits, PR's, and discussions.


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

I'm not sure that a patch review will get greater visibility than a thread
on llvm-dev. If you feel strongly I can do this though.

-- Sean Silva


>
>
>>
>> -- 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/173fe7ab/attachment.html>