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

[Anna Thomas via llvm-commits]

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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180103/d86a0c6f/attachment.html>