[SimplifyCFG] Hoist common if-then-else code if used by two-entry PHI nodes
Needs ReviewPublic

Authored by labrinea on Jun 13 2018, 6:33 AM.

Details

Summary

The current algorithm for hoisting if-then-else code is limited to only match identical instructions with the exact same ordering. The reason is to keep the complexity low. There are cases where the InstCombiner cannot sink the common code and we end up with trivial PHIs which cannot be eliminated. This prevents if-conversion. SimplifyCFG is not the best place for this kind of transformation but it is a short-term solution until GVN-Hoist gets fixed/enabled.

Diff Detail

labrinea created this revision.Jun 13 2018, 6:33 AM

I'm confused ... Is there a reason to not just go fix/enable gvn-hoist?

My argument would be that this change is fairly small. To be honest I don't know what is the status of GVN-Hoist. 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.

Okay, i'll bite

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.

GVNHoist is generally pretty stable; my team has used it to build a lot of code without any issues. There are a few known bugs (https://bugs.llvm.org/show_bug.cgi?id=37791, https://bugs.llvm.org/show_bug.cgi?id=36635, https://bugs.llvm.org/show_bug.cgi?id=37445 ) that we probably need to fix before turning it on by default.