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

Tue Aug 8 05:34:19 PDT 2017

grosser added a comment.

In https://reviews.llvm.org/D35851#835026, @chandlerc wrote:

> In https://reviews.llvm.org/D35851#829720, @grosser wrote:
>
> > I need to think about these. Some LLVM-IR unit tests that explain the property you are looking for would make the discussion here more concrete, as I will need some time to translate your textual ideas to IR. Even without, I certainly will have a look at your pointers.
>
>
> <snip>
>
> > Overall, I am really not trying to shoot down this proposal. Even though I would like to have a PDT that works well with regions, if we can come up together with good test cases that show this is not possible, I am the first to commit and document them! Please help me to get your requirements written into nice understandable unit tests! If our discussions end up in useful test cases and documentation, then it was certainly of use.
>
> <snip>
>
> > Again, I will need to think about the above and will try to come up with LLVM-IR test cases that illustrate the problem you describe. If you happen to have some available, this would be very much appreciated.
>
> Hey Tobias,
>
> Jakub mentioned that he was essentially blocked here and there didn't seem to be a good path forward, and I have to agree that this doesn't seem like a very tenable end state...

I fully agree with you. We should get this as quickly resolved as possible!

> I see two high level issues here:
> 
> 1. You've somewhat left this review in a bad place. "I need to think about this" without a real time bound doesn't seem like a tenable state. It's been almost a week without update. I think it is very reasonable for folks to want to make progress without getting blocked like this.

I also would like to move on, especially as I would like Jakub to be able to close this last point (and also because it takes a lot of my time). I am doing my best to look into this. It would really help if we can boil this down to some LLVM-IR test cases and which we can use to better document the guarantees the proposed post-dominator implementation gives. I now need to find a time slot to translate Dany's examples to LLVM-IR. If I could get help with this, this would be amazing.

> 2. You seem to be setting the bar *really* high. I know that regions are used in Polly, but they aren't used in any other part of LLVM and postdom has fairly immediate uses. While I'm also interested in figuring out the right way to integrate things like Polly's regions with postdom, I don't think it is really reasonable to hold up pretty significant improvements to the rest of LLVM on that working. I feel like we should be able to make more incremental progress here.

While this tangentially also affects Polly (for infinite loops), we can certainly work around this. Dany even suggested to have a Polly compatible mode.

Now, my main concerns are not regions or Polly, but the semantics of post-dominance in general. We are implementing an extension which is not documented in papers (even though other similar implementations exist). I would like to get the guarantees and implications documented before we move large portions of LLVM to use this.

My main concerns is the fact that the introduction of virtual edges weakens the post-dominance relationship. If we want to verify the correct placement of life-range metadata one could e.g. require that the life-range-end node post-dominates the live-range start. This invariant could be verified throughout the pass transformations to ensure no pass breaks this invariant. Unfortunately, when cfg-simplify drops edges and we re-connect the backwards unreachable nodes to the exit node as proposed here, the post-dominance relation is weakened and the verification surprisingly fails. I would really like to understand (and see documentation) if writing such checks 
with the proposed post-dominance information would require us to think about such possible invalidations at each point where we potentially make parts of the CFG backwards unreachable.

This is not the case when working with a complete CFG (or a CFG where unreachable nodes are not modeled).

> My suggestion would be something like the following:
> 
> - If this actively breaks some Polly use case, we should get a test case for that *now*. And if so, it might make sense to have Polly use its own postdom code until the interactions here are sorted out. There doesn't seem to be any need for one singular implementation of this stuff.

There are test cases in tree since years, which I believe break. What else can I provide?

I clearly dislike two versions, but even if we indeed introduce two versions their guarantees and semantics differences should be made clear.

> - Jakub and Danny make progress on postdoms here, specifically making them faster to update and more viable for some of the non-region use cases.

I totally agree. This is great in general and I am very glad to see this going in. Jakub did an outstanding job here.

> - If/when folks have a better idea of how to integrate them with regions *or* an understanding of why they can't *then* we add the region test cases and support you're mentioning.

> Essentially, 100% integration with regions, an analysis with a very specialized single user at the moment, doesn't seem like a great hard requirement for us to make progress. While I don't want to break regions and Polly, I think these things need to be able to move independently rather than being forced to boil the ocean.

Polly won't break with this change, but it will be very difficult to asses alternatives if the current requirements are not documented and tested.

Let me propose a way forward:

- I discussed this very same issue with Hal at the LLVM summer school for over two hours. He has very good understanding of this subject. If he can review this patch faster then me and agrees with the direction, I am glad to accept to step back from this review.

- I really would like to get at least one (or two) LLVM-IR test cases that clarify the corner-case properties of the proposed patch. If Jakub could help me to translate Dany's example into a self-contained test case, this would be outstanding. This would make it easier for me to reply. Even if not, I will try to look into this tonight or tomorrow night at the latest and will propose Dany's example as new test case. This will then hopefully give more insights.

Best,
Tobias

https://reviews.llvm.org/D35851