[PATCH] D52827: [LICM] Make LICM able to hoist phis

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.

I don't really have strong objections against that. Thanks for your explanation. It could've been a Phi node insertion instead of rehoisting in this case, but I don't really see why rehoisting should be bad (other than we may end up executing code that never executes, but it's a general LICM flaw and not specific to this patch). I also see no bug in what we have now. So I'm ok with the idea.

I am not comfortable with reverting it back and forth, so I will request some re-design of your patch. Please make an option to switch off/on CFG hoisting. We will need it if this transform will expose some unexpected bugs in the future. Reverting patch that big may become hard in time. Then, I'm OK if you check in the current patch with this option switch by default (in tests, it can be set to true).

Then, you can check in a patch that turns it on by default.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D52827/new/

https://reviews.llvm.org/D52827