[PATCH] D32740: [PM/Unswitch] Fix a bug in the domtree update logic for the new unswitch pass.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D32740#753141, @dberlin wrote:

> I have an intern starting in two weeks to do incremental dominator algorithms :)


Very excited to move this code to something better. But I'd also like to make progress here.

> 
> 
> In https://reviews.llvm.org/D32740#752838, @chandlerc wrote:
> 
>> In https://reviews.llvm.org/D32740#748894, @davide wrote:
>>
>> > The algorithm you have, I think, it's correct to start (although as pointed out by Sanjoy doesn't really compute the dominance frontier).
>> >  Have you considered computing the iterated dominance frontier every time instead? If there are technical difficulties (or it's just slower) I'd add a comment to the code explaining why we can't use it.
>>
>>
>> Added some comments about why the existing IDF calculator seems like a bad fit for what we want here.
> 
> 
> TBQH, both of the comment seem quite wrong.
> 
> 1. Yes it computes dom tree levels each time. This is trivially fixable, but historically has never been a time sink.
> 2. You say:
> 
>   ``` //  It includes extra complexity and logic to allow PHI placement which is //    somewhat fundamentally a harder problem than the one we're trying to //    solve here, so we can get away with a simpler approach. Specifically, //    we don't need to do any live-in pruning.
> 
>   ``` No it isn't and no it doesn't :) At least, the issues you describe are ... not a good reason to avoid it. The problem of proper phi placement  is not "fundamentally harder ". it's quite literally identical. :) Second, the live-in pruning is completely optional, and costs nothing when it is off. As an example, MemorySSA does not use the live-in pruning.
> 
>   If you do not set the live in blocks, it simply does not use them. This is why the comment says "/// By default, liveness is not used to prune the IDF computation."
> 
>   You also could use them in your case if you wanted a smaller and more optimal answer.

Ok, I've updated the comments. I agree these are fixable issues, but they're still issues. I'm happy to refactor this to share code if that's useful and I've updated the comment to reflect that we should factor things in a way that makes it easy to share.

The issue I was *trying* to get at in the second point was that the API isn't terribly convenient for this use case (but looks very much like the API that I'd want for PHI placement). Not a comment on the actual logic, I completely agree that could be shared. I have a mild preference to get the other domtree updates I'm going to need to do in this code written to better understand the API before rewriting the API used to access the IDF calculator logic just to have a better idea. And it isn't a lot of code (30 lines including lots of comments). But if you're really worried about having the logic local, I can plumb through the IDF interface bits.

> (The mechanism you are currently using will go badly N^2 on certain types of CFG's)

Is this different from IDF calculator? Is that difference the domtree levels? If it is some other difference, I don't really understand it, and would appreciate an explanation. It's possible I'm just missing it, but I'm not seeing how the two meaningfully differ.

If it is just the levels, I'm not sure how to address this without adding a different N^2 due to walking the entire domtree rather than just a localized region. At that point, i'd rather just recalculate at the end of unswitching.

-Chandler


https://reviews.llvm.org/D32740