[PATCH] D38566: [SimplifyCFG] don't sink common insts too soon (PR34603)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D38566#952038, @hfinkel wrote:

> In https://reviews.llvm.org/D38566#944355, @spatel wrote:
>
> > Ping * 2.
>
>
> Can you please summarize where we are now with this? I still prefer to have only one form of SimplifyCFG during canonicalization. I feel that doing otherwise is a mistake, in part to make the canonical form understandable, and in part because canonicalization is iterative (now for devirtualization, but maybe in other ways in the future), and so it's much better if we don't think about phases within canonicalization itself.


Sure - the patch is identical to when we discussed this around Oct 30,31. The preliminary steps were blamed for causing a bug ( https://bugs.llvm.org/show_bug.cgi?id=35210 ) and reverted, but subsequently the patches were exonerated, and the bug was fixed independently of this.

There was also concern that having options to passes makes it harder to track down bugs and is a sign of bad design. I don't disagree with that view, but we've already come this far, so I view a change of direction as a follow-up because that's significantly more work at this point. Also, this patch fixes a known regression (a significant performance hit), and I want to close that hole ASAP.

So we have the bare minimum to avoid the bug in the motivating case. But I see your point - having multiple canonical forms/phases will make things more complicated. I thought I might have a test case for the larger patch, but I haven't found an example yet. I'll update the patch as you've requested. If I can come up with a test for it, I'll include it. If not, hopefully, we can just proceed with the one test that's already here.


https://reviews.llvm.org/D38566