[PATCH] D146792: [LoopPredication] Do not sink conditions to branches. PR61673

[Anna Thomas via Phabricator via llvm-commits]

anna added a subscriber: apilipenko.
anna added a comment.

Adding more context here: This transform gets enabled by LICM hoisting widenable conditions (today it is hidden because we stopped LICM from hoisting widenable conditions: https://reviews.llvm.org/D146274). Also, while discussing with @apilipenko  about the comment in Loop Predication, he pointed that the latter part of the comment referred to older implementation in loop predication before we had GuardUtils::widenBranch API.
Specifically the latter part of this comment (" inserting code before it and then modifying the operand is legal.") :

  // Subtlety: We need to avoid inserting additional uses of the WC.  We know
   // that it can only have one transitive use at the moment, and thus moving
   // that use to just before the branch and inserting code before it and then
   // modifying the operand is legal.

But we could not identify why "we need to avoid inserting additional uses of the WC".

It seems there is a wider problem here. At least, according to me, there is nothing in the semantics of widenable condition that prevents us from sinking widenable condition into the loop. This means we can have subtle (and hard to debug) miscompiles such as the one caused here and in GuardWidening (explained here: https://github.com/llvm/llvm-project/issues/60234). Anything that prevents LLVM transforms from sinking widenable condition into loops?

One more point: the original cause of the downstream miscompile is still present after this fix (that makes sense because even when we sink the widenable condition into the loop, later LICM comes along and hoists it out, so the bug attempted to be fixed by this patch is masked). In short - the miscompile is seen when we have loop predication's hoisting of widenable condition, followed by loop predication's predicate loop exits transform here. Will continue investigation on that front.

This patch probably solves a bug as explained by Max here: https://github.com/llvm/llvm-project/issues/61673


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

https://reviews.llvm.org/D146792