[PATCH] D48122: [SimplifyCFG] Hoist common if-then-else code if used by two-entry PHI nodes

[Daniel Berlin via Phabricator via llvm-commits]

dberlin added a comment.

Okay, i'll bite

In https://reviews.llvm.org/D48122#1131183, @labrinea wrote:

> My argument would be that this change is fairly small.


It's also not like SimplifyCFG started out large.
Like all things, it does so much because of small changes accumulating without a real plan :)

>   To be honest I don't know what is the status of GVN-Hoist. 

I"m only aware of a single bug ATM.

> If it is in a good shape, then enabling it is the best way to go. If there are a few bugs to fix I am happy to help, but if it is far from being in a good shape then a workaround like this should be acceptable.

At least to me, the shape is not so important as what the plan is.
If you are saying things like "this should go away when gvnhoist is fixed/enabled", that should really be part of a plan, and we should have that plan first.  Based on the timelines in that plan, we should be deciding whether or not we want a short term workaround.

It doesn't have to be super formal or super-correct, but we should at least understand what the state and what we want it to be  before we decide whether to add workarounds.


https://reviews.llvm.org/D48122