[PATCH] D35851: [Dominators] Include infinite loops in PostDominatorTree

[Tobias Grosser via Phabricator via llvm-commits]

grosser added a comment.

In https://reviews.llvm.org/D35851#836313, @grosser wrote:

> Add Dani's email reply to Phab:
>
> >> If you generate that, you will eliminate the store a, 5 again, and you should not.
> > 
> > Interesting, but slightly confusing.
>
> This is what i get for trying to reduce an algorithm quickly.


That's OK. It gives us an example we can work with. At least we are now on the same page. We still need to correct the example and identify the property you are interested in, but I feel we make progress. Thanks a lot again!

> I'm sure i incorrectly removed some postdominance frontier checks or something.

I doubt a post-dominance frontier check can be the reason. The post-dominance frontier is based on the PDT, so it should be identical for the two examples.

> It's probably easiest to take one of the working PDSE patches and look at that.

AFAIK, you have been working closely with the PDSE people. The patches seem still WIP, so I will have a harder time digging through them to find the part that matters. Any chance you happen to have one of the authors in the office, such that you can ask him/her over lunch? 
  

>> Assume you change the previous program slightly:
>> 
>>     define void @foo(float* %a) {
>>        ....
>>     }
>>   
>>     define void @foo(i1 %cond, float* %a) {
>>     block1:
>>        store float 5.0, float* %a
>>        br i1 %cond, label %block2, label %block3
>>   
>>     block2:
>>       call foo(float* %a)
>>       br i1 true, label %block2, label %block1   <---- Only this line has changed
>>   
>>     block3:
>>       store float 6.0, float* %a
>>     }
>>   
>>   You get a CFG very similar to the one I posted earlier:
> 
> 
> 
>> https://reviews.llvm.org/F3887544: Post Dominance.png https://reviews.llvm.org/F3887544 
>>  In your case the PDT is the first of the three you just mentioned:
> 
>  
> 
>>> 3
>> 
>>   >
>>   >  |
>>   >
>>   >
>>   > 1
>>   >
>>   >  |
>>   >
>>   >
>>   > 2
>>   
>>   This is the post-dominator tree which we compute today and which won't be changed by this proposal (as no reverse-unreachable blocks exist).
> 
> This program should be illegal, and as far as i can tell, it is :)
> 
> -> % bin/opt -view-cfg foo2.ll
>  Entry block to function must not have predecessors!
>  label %block1
>  bin/opt: foo2.ll: error: input module is broken!
> 
> Let's assume that's trivial to fix though, because it is.

LLVM does not allow jumps to the entry node. Jakub fixed it correctly by adding an auxiliary block that does nothing but jumping to block1.

>> One other slightly related question. Do you assume that A post-dominates B means that a program that reaches B eventually must reach A?
> 
> You can't say anything block wise, only edge wise.

I am confused. Post-dominance is defined on BBs and paths, so the above should make sense. To me it seems to be where the confusion comes from. To my understanding A post-dominates B does not guarantee that a program that reaches B eventually must reach A. In all your previous examples, you assumed otherwise. For PDSE to be correct, this cannot be assumed there.

> That is, C is dependent on edge A->B if C postdominates B and C does not strictly postdominate A.

That seems to be the definition of control dependence. The properties we can take from this depend on what post-dominance guarantees.

In https://reviews.llvm.org/D35851#836314, @grosser wrote:

> Added Dani's email reply II to phab:
>
> So, i definitely removed a postdominancefrontier check.
>  The PDF of X is equivalent to the set of branch blocks upon which X is control dependent.
>  Though, as mentioned this only means the block controls whether X executes, you need to label the edges.  
>
> FWIW:I noticed that earlier, you mentioned literature does not talk about what we are doing, though there are practical implementations. It's worth pointing out that literature doesn't often talk about it as part of the postdominator construction itself, but as part of the CFG part that they build post-dom on top of.  They very often assume and require a unique exit node where everything has a path to it. 
>
> Here are some example seminal papers that do this, and then build post-dom/post-domfrontiers on top of it:
>
> Cytron's Control dependence paper:
>  http://polaris.cs.uiuc.edu/~padua/cs426/p451-cytron.pdf page 456. 
>  Note that they require everything have a path to exit:
>  "We assume that each
>  node is on a path from Entry and on a path to Exit"
>
> Ferrante's program dependence graph paper
>  http://dl.acm.org/citation.cfm?id=24041
>  "Definition 1. A control flow graph is a directed graph G augmented with a
>  unique entry node START and a unique exit node STOP such that each node in
>  the graph has at most two successors. ...  We further assume that for any node N in G there exists a path
>  from START to N and a path from N to STOP. "
>
> If you start with a CFG where you require every node go to exit, you don't need to do anything special just when computing postdom, because you just added the previously virtual edges to the actual CFG instead :)


Very right, this phrase is very common (and basically the indication that they did not think about incomplete CFGs at all). This is why none of the above papers directly applies. We always first need to fix the CFG, and then can use the papers. How we do this fix is something we need to show is correct and does not break earlier assumptions.

Following Chandler's suggestion, here an idea of a path forward:

1. Let's nail the PDSE thingy. We have a test case we all understand and a transformation that does something useful. Let's use this to identify the PDT property you need (and others that may not be needed) and write this down in the documentation. We now had two examples which we first believed work and then it became clear that just checking the PDT is not enough. If we all get confused by this, how can a newcomer not need documentation here.

2. Depending on the outcome of 1) we can either already clearly rule out my alternative suggestion or show that they indeed would generally not break the PDSE algorithm.

I hope to converge quickly on 1) and 2). Even if we agree that my alternatives would not break things immediately, we should still judge their usefulness. I believe that by (commonly) not breaking the PDT in the reverse-reachable graph, they have some benefit. However, you still have the earlier code complexity argument on your side. I would love to see to have you at least evaluate these options  (considering the outcome of our discussion) as I believe the needed changes are small. However, I clearly admit that in this case Jakub and your thoughts about code complexity are very important, so should have a strong weight in this discussion.


https://reviews.llvm.org/D35851