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

[Sean Silva via llvm-commits]

On Mon, Aug 8, 2016 at 12:42 AM, Sean Silva <chisophugis at gmail.com> wrote:

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

I've gone ahead and reopened the discussion in the thread "Should analyses
be able to hold AssertingVH to IR? (related to PR28400)". Feel free to
comment there.

-- Sean Silva


>
> -- 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/Transform
>>>>> s/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/b0834ab1/attachment.html>