[llvm] r321653 - [BasicBlockUtils] Check for unreachable preds before updating LI in UpdateAnalysisInformation

Philip Reames via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 5 11:10:35 PST 2018 

On 01/03/2018 02:20 PM, Daniel Sanders wrote:
> Hi Philip,
>
>> On 3 Jan 2018, at 13:31, Philip Reames via llvm-commits 
>> <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>
>> Volkan,
>>
>> If you are requiring loop info, you are computing domtree.  You can 
>> add a requirement for domtree and pass it in at no additional runtime 
>> cost.
>
> I agree, I'm not sure why it's currently optional in our downstream 
> pass but this would certainly fix the problem.
>
>> Moreover, Anna has made a perfectly reasonable change.  We do not 
>> restrict upstream development to prevent breaking downstream work.  
>> We're happy to help guide you in migrating your downstream code, but 
>> you should not expect that downstream breakage (even if it wasn't 
>> easy to fix) would be a cause for an upstream revert.  It is not.
>
> I don't believe Volkan requested that the patch be reverted so I'm not 
> sure where the harsh tone is coming from. Is there something I've missed?
Let me apologize for the tone.  I hadn't intended to come across as 
harsh here, but re-reading I see that I definitely did.  Sorry.

My concern was that we had a request to change an implementation 
strategy due to effects on downstream code.  We generally don't want to 
be taking on the maintenance burden for keeping out of tree code 
working.  We do in selective cases where the burden is small and clearly 
motivated, but as of the time of my response, I was not convinced this 
example met that bar.

I believe the rest of this is discussed downthread, so I'll leave it there.

>
> As far as I can see, there's a belief that 
> addRequired<LoopInfoWrapperPass>() ensures that the domtree is 
> available but while that's often the case it isn't quite guaranteed as 
> the pass execution log Volkan provided shows. That said, 
> calling UpdateAnalysisInformation() without having a domtree to update 
> is strange at best (the comment says it updates the domtree and you 
> can't update something you don't have) and as Anna explained you need 
> the domtree to update the loop info anyway.
>
> I currently think that we should something like:
> assert(DT && "DomTree must be available to update loop info");
> to the code so that passes that update the loop info but didn't ensure 
> the dom tree is available with their own 
> addRequired<DomTreeWrapperPass>() get a better hint as to what they 
> need to do.
>
>> Philip
>>
>> On 01/03/2018 01:15 PM, Volkan Keles via llvm-commits wrote:
>>> Hi Anna,
>>>
>>> As you can see in my previous e-mail, it may be freed if it’s not 
>>> required later. It’s required to compute LoopInfo, but it’s not 
>>> required to use LoopInfo. In UpdateAnalysisInformation, DT is not 
>>> only updated but also used, so it’s no longer optional. I think we 
>>> should make it a reference or at least check if it’s provided or 
>>> not. Does that make sense?
>>>
>>> Volkan
>>>
>>>> On Jan 3, 2018, at 12:35 PM, Anna Thomas <anna at azul.com 
>>>> <mailto:anna at azul.com>> wrote:
>>>>
>>>> Hi Volkan,
>>>>
>>>>
>>>>> On Jan 3, 2018, at 2:31 PM, Volkan Keles <vkeles at apple.com 
>>>>> <mailto:vkeles at apple.com>> wrote:
>>>>>
>>>>> Hi Anna,
>>>>>
>>>>> Yes, LI is computed using DT, but this doesn’t mean DT is required.
>>>> If LI is available for a pass at a particular point, it means DT is 
>>>> available at that point as well (or can be retrieved).
>>>> This is because the required pass for LI is the DomTreeWrapperPass. 
>>>> The DT pass will only be free’d when the LoopInfo pass is free’d.
>>>> For example, lets say we wanted to recompute LI using the analyze 
>>>> function - we need to pass in a dom tree.
>>>>
>>>> Could you pls verify if this is true in your case and if so, it 
>>>> will provide a quick and correct fix for your pass.
>>>>
>>>>> Here is what llc with the option `-debug-pass=Executions` produces:
>>>>>
>>>>> [2018-01-03 11:24:29.528119000] 0x7f90546035a0     Executing Pass 
>>>>> 'Dominator Tree Construction' on Function ‘foo’…
>>>>> …
>>>>> [2018-01-03 11:24:29.528212000] 0x7f90546035a0     Executing Pass 
>>>>> 'Natural Loop Information' on Function ‘foo’…
>>>>> …
>>>>> [2018-01-03 11:24:29.528834000] 0x7f90546035a0      Freeing Pass 
>>>>> 'Dominator Tree Construction' on Function ‘foo’…
>>>>>
>>>>> So, DominatorTree may not be available at this point. I think DT 
>>>>> shouldn’t be optional in this function if it’s required.
>>>> Neither DT nor LI is required by this function - if neither is 
>>>> provided, we don’t do anything in UpdateAnalysisInformation.
>>>> We should be more strict here and check that if LI is being asked 
>>>> to be updated, we need DT as well. This will unbreak your bot
>>>> but will no longer preserve LI unless DT is passed.
>>>>
>>>> This seems like a valid thing to do given the reasoning above (DT 
>>>> is available as long as LI is available).
>>>>
>>>>
>>>> Anna
>>>>>
>>>>> Volkan
>>>>>
>>>>>> On Jan 3, 2018, at 6:09 AM, Anna Thomas <anna at azul.com 
>>>>>> <mailto:anna at azul.com>> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jan 2, 2018, at 10:06 PM, Friedman, Eli 
>>>>>>> <efriedma at codeaurora.org <mailto:efriedma at codeaurora.org>> wrote:
>>>>>>>
>>>>>>> On 1/2/2018 3:45 PM, Volkan Keles via llvm-commits wrote:
>>>>>>>> +llvm-commits
>>>>>>>>
>>>>>>>>> On Jan 2, 2018, at 8:25 AM, Anna Thomas via llvm-commits 
>>>>>>>>> <llvm-commits at lists.llvm.org 
>>>>>>>>> <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>>>>>>>
>>>>>>>>> Author: annat
>>>>>>>>> Date: Tue Jan  2 08:25:50 2018
>>>>>>>>> New Revision: 321653
>>>>>>>>>
>>>>>>>>> URL:http://llvm.org/viewvc/llvm-project?rev=321653&view=rev
>>>>>>>>> Log:
>>>>>>>>> [BasicBlockUtils] Check for unreachable preds before updating 
>>>>>>>>> LI in UpdateAnalysisInformation
>>>>>>>>>
>>>>>>>>> Summary:
>>>>>>>>> We are incorrectly updating the LI when loop-simplify generates
>>>>>>>>> dedicated exit blocks for a loop. The issue is that there's an 
>>>>>>>>> implicit
>>>>>>>>> assumption that the Preds passed into 
>>>>>>>>> UpdateAnalysisInformation are
>>>>>>>>> reachable. However, this is not true and breaks LI by incorrectly
>>>>>>>>> updating the header of a loop.
>>>>>>>>>
>>>>>>>>> One such case is when we generate dedicated exits when the 
>>>>>>>>> exit block is
>>>>>>>>> a landing pad (through SplitLandingPadPredecessors). There 
>>>>>>>>> maybe other
>>>>>>>>> cases as well, since we do not guarantee that Preds passed in are
>>>>>>>>> reachable basic blocks.
>>>>>>>>>
>>>>>>>>> The added test case shows how loop-simplify breaks LI for the 
>>>>>>>>> outer loop (and DT in turn)
>>>>>>>>> after we try to generate the LoopSimplifyForm.
>>>>>>>>>
>>>>>>>>> Reviewers: davide, chandlerc, sanjoy
>>>>>>>>>
>>>>>>>>> Reviewed By: davide
>>>>>>>>>
>>>>>>>>> Subscribers: llvm-commits
>>>>>>>>>
>>>>>>>>> Differential Revision:https://reviews.llvm.org/D41519
>>>>>>>>>
>>>>>>>>> Modified:
>>>>>>>>>    llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp
>>>>>>>>>    llvm/trunk/test/Transforms/LoopSimplify/unreachable-loop-pred.ll
>>>>>>>>>
>>>>>>>>> Modified: llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp
>>>>>>>>> URL:http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp?rev=321653&r1=321652&r2=321653&view=diff
>>>>>>>>> ==============================================================================
>>>>>>>>> --- llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp (original)
>>>>>>>>> +++ llvm/trunk/lib/Transforms/Utils/BasicBlockUtils.cpp Tue 
>>>>>>>>> Jan  2 08:25:50 2018
>>>>>>>>> @@ -323,6 +323,12 @@ static void UpdateAnalysisInformation(Ba
>>>>>>>>>   bool IsLoopEntry = !!L;
>>>>>>>>>   bool SplitMakesNewLoopHeader = false;
>>>>>>>>>   for (BasicBlock *Pred : Preds) {
>>>>>>>>> +    // Preds that are not reachable from entry should not be 
>>>>>>>>> used to identify if
>>>>>>>>> +    // OldBB is a loop entry or if SplitMakesNewLoopHeader. 
>>>>>>>>> Unreachable blocks
>>>>>>>>> +    // are not within any loops, so we incorrectly mark 
>>>>>>>>> SplitMakesNewLoopHeader
>>>>>>>>> +    // as true and make the NewBB the header of some loop. 
>>>>>>>>> This breaks LI.
>>>>>>>>> +    if (!DT->isReachableFromEntry(Pred))
>>>>>>>> Hi Anna,
>>>>>>>>
>>>>>>>> This change breaks our internal bots because DT might be 
>>>>>>>> nullptr as it is optional. Is there another way to check this?
>>>>>>>
>>>>>>> This code only executes if the LoopInfo pointer is non-null, and 
>>>>>>> LoopInfo requires a DominatorTree to compute, so you probably 
>>>>>>> have a domtree somewhere even if your pass doesn't explicitly 
>>>>>>> require it.
>>>>>> Agree with Eli here. If LI is being passed in to this method, it 
>>>>>> makes sense to pass in the DT as well (which exists because LI 
>>>>>> was computed using DT).
>>>>>> Reachability analysis without DT would be from first principles 
>>>>>> because LI does not contain that information.
>>>>>>
>>>>>>
>>>>>> Anna
>>>>>>>
>>>>>>> -Eli
>>>>>>>
>>>>>>> --
>>>>>>> Employee of Qualcomm Innovation Center, Inc.
>>>>>>> Qualcomm Innovation Center, Inc. is a member of Code Aurora 
>>>>>>> Forum, a Linux Foundation Collaborative Project
>>>>>
>>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto: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/20180105/5b096ee9/attachment.html>

More information about the llvm-commits mailing list