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

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

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

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


Sadly, no. They don't work for me or Google :)
The only other reference code for PDSE that i'm aware of is Open64, which is "hard to follow" to say the least.

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

Sorry, let me be clear.
You can define it block wise, but it requires more than the PDT alone, as you must look at successors of the block.

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

I have not assumed otherwise :)

> For PDSE to be correct, this cannot be assumed there.

On this we agree.

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

I believe at this point, what you want is documented?
If not, can you point out what is not?

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

This is the part i disagree with.
Assume for a second your suggestion is an improvement, and in fact even an amazing one!.
We have already shown what we are doing here is conservatively correct.

If your suggestion is an improvement, and we can later prove it, awesome. Let's do that.

But given that the majority of this patch is unrelated to your suggestion, and this patch can be shown to work, why isn't the right answer:
Commit this patch, if you can improve it in a followup, improve it!

In fact, in pretty much every other review i've seen, we've explicitly asked people to separate followup improvements.

I think we should do the same here.
I get the distinct impression, given how hard you are pushing not to follow this path, that you are  worried that if we commit this patch, that will not happen.
But i really don't understand why.


https://reviews.llvm.org/D35851