[PATCH] D103316: Hoist llvm.assume into single predecessor if block otherwise empty

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

Not sure I've actually captured all the logic in how we ended up here, considering the original problem was that CodeGenPrepare is too pessimistic when it removes all llvm.assume intrinsics when doing CFG transformations.

The first patch from @markus aimed at improving CodeGenPrepare by making it a bit less aggressive and keeping some llvm.assumes. And then there was an idea to make it even less aggressive by also salvaging some llvm.assume intrinsics.

Now, we have ended up with a patch modifying SimplifyCFG instead. Which obviously doesn't solve the problems we have in CodeGenPrepare. So what is the ultimate goal with this approach? Is the idea that we should remove the CFG transformations in CodeGenPrepare (at least eliminateMostlyEmptyBlocks?) in a follow up patch? Or is the idea to reuse these Utils helpers in CodeGenPrepare when doing such CFG transforms?

I mean, currently the CFG tranformations in CodeGenPrepare might depend on the existence of llvm.assume in the code. That problem still exist unless we also do something in CodeGenPrepare as well, given that the ultimate goal still is that we want to keep/salvage as many llvm.assume intrinsics as possible until ISel.
According to code comments in CodeGenPrepare those transforms are done (at least partially) as a cleanup after early IR passes in codegen such as LSR. And not all targets are running SimplifyCFG between LSR and CodeGenPrepare. Maybe the idea is to also add SimplifyCFG to the codegen pipeline just before CodeGenPrepare as a preparation to remove CodeGenPrepare::eliminateMostlyEmptyBlocks?


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

https://reviews.llvm.org/D103316