[llvm] r296535 - Fix PR 24415 (at least), by making our post-dominator tree behavior sane.

[Daniel Berlin via llvm-commits]

On Thu, Mar 2, 2017 at 12:10 PM, Tobias Grosser <tobias.grosser at inf.ethz.ch>
wrote:

> On Thu, Mar 2, 2017, at 07:54 PM, Daniel Berlin wrote:
> > (and, fwiw, i wrote this in a rush because i'm on vacation and running
> > around, but wanted to make sure i responded.
> > I didn't edit it very hard, so please don't assume I mean anything mean
> > by
> > saying "non-standard", etc, in a bunch of places :P)
>
> Hi Dani,
>
> I don't think there is a clear disagreement yet, I mostly want to get
> time to carefully think about all implications. I know you have (and
> there are) good reasons and arguments to go this way. Still, I believe
> some of the arguments are not as black and white as you point them out
> at the moment and not all implications / choices have been discussed
> (certainly not with me)  and documented. Maybe they are indeed obvious
> and just not clear to me, but in this case maybe we can also use this
> opportunity to better document the choices we took and the guarantees we
> give.
>

Sure.


>
> As you seem on vacation, there does not seem to be a huge urge to get
> this change in (especially as the current status was there for almost
> ten years). I will take the weekend to look into this again in great
> detail, also taking into account the good arguments you give, the other
> bug-reports that are affected, ...
>

Sounds good to me.
Now that i have a second:

There are two problems i see:

1. I don't believe it is possible to satisfy the normal path invariants of
the post-dom tree with infinite loops without connecting them to a block
that goes to virtual exit.
If you can think of a way, awesome.  I'm pretty sure, but not positive,
that it's not possible.  I know Tarjan didn't think it was possible, and
he's a lot smarter than me :)

2. The other underlying problem is that reverse-unreachable is different
than forward-unreachable.
We can legally just ignore all forward-unreachable things, and delete them,
or otherwise not care about them, because they can never execute.
Reverse-unreachable things can still execute, so we can't just ignore
them.  If we can't ignore them, and we want to form a post-dominator tree
that includes them, we run into the first problem.

The options i can think of:
1. Given an option to ignore reverse-unreachable  and build an analysis
name for that.
Note that we can actually do this in a more complete way than we did
pre-patch. post-patch, we properly detect *all* which things do not reach
program exit (instead of just some of them) and so we could simply not
connect them anywhere, and you will get a post-dom tree that doesn't
include them (and isn't "flattened")
This would be more "correct" than what we did pre-patch, in the sense that
you would get a post-dom tree without infinite loops in it, instead of
"with some types of infinite loops ending up in weird places, and other
types of loops and noreturns completely ignored".

Note: This is also more correct in that the current region building code
assumes the path invariants of the dom-tree and post-dom tree hold, and
thus included, in a region, things that appear on a walk from one node to
another (regardless of where the would otherwise appear in the dom/post-dom
tree)

So in that sense, removing things from both trees and building regions
should give you regions that don't include those nodes, and would be
correct if those nodes did not exist (as opposed, again, to pre-patch,
where what you get depends on the CFG structure of the  infinite loop or
noreturn that existed, etc)

2. (and i don't know if this helps) Create a second virtual node post-dom'd
by virtual exit the represents all the fake-exits.
This would let you ignore them all at once, but still leaves a "flattened"
post-dom tree.

IE

virtual exit
  /                                          |
virtual infinite loop exit         each normal returning node as a sibling
   |
each infinite loop backedge as a child


The problem is the dom-tree will still be "flatter" overall for reachable
nodes.
Note: It's already the case, that post-patch, any direct child of virtual
exit not ending in a return must be fake, so this would just make it
slightly easier.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170302/f401e0b7/attachment.html>