[PATCH] D102615: [LoopDeletion] Break backedge if we can prove that the loop is exited on 1st iteration (try 3)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D102615#2804481 <https://reviews.llvm.org/D102615#2804481>, @mkazantsev wrote:

> SCEV is much more powerful in terms of simplification. Imagine the situation when some Phi takes value of `a + b + c` on the 1st iteration (and unknown on all others), then gets subtracted by `a`, then by `b`, and then by `c`, and then the result is compared against zero. If we want `InstSimplify` to do something like this, we will need to re-implement recrusive expression aggregation, simplification, in summary - we will need to re-invent SCEV.
>
> I'm also sceptical about the approach that we are following (cripple the opts to save compile time). I'd rather prefer it to be `isKnownPredicateAt`, maybe under a flag, to use as much power as it can.
>
> If the SCEV's CT impact is still unacceptable, we can cripple it even further to use `isKnownViaNonRecursiveReasoning`, but basically in this case we can just go with `-O0` and stay happy about the results.

Let me start with a bit of philosophy: This patch falls into the very unfortunate category where a) it solves some specific problem you have, b) outside that, it applies very rarely, but c) has a compile-time impact on everyone. It's easy to justify an average-case compile-time impact that shows an average-case improvement. It's not so easy to justify an average-case compile-time impact that only shows an exceptional-case improvement.

To provide some numbers: After adding a `loop-delete.NumBackedgesBroken` stat and running test-suite at `O3`, the stat increases from 94 to 96. That is, in the entirety of test-suite, we break two additional back edges. Of course, test-suite is skewed in the kind of IR it contains (being C/C++), but this gives us a general idea.

I do think it's okay to solve specific optimization problems that do not have any wider applicability, but we should try to minimize the impact they have on everyone else. I do understand that making this SCEV based can in principle handle more cases, but as always, there are tradeoffs. I gave an InstSimplify based implementation a try (quick hack: https://github.com/llvm/llvm-project/commit/5aa9bbc50a1fed1364caaa7edc9273fb38de990d), and this had the same practical optimization power on test-suite (2 additional backedges broken) as the SCEV-based implementation. Only difference is that the SCEV-based implementation caused codegen changes on two additional test cases without breaking a backedge, due to some kind of SCEV caching difference. (Compile-time for that version is https://llvm-compile-time-tracker.com/compare.php?from=8fdd7c2ff16da370e28ef1b22e400d57a541484f&to=5aa9bbc50a1fed1364caaa7edc9273fb38de990d&stat=instructions)

Taking a step back here from the technical details, I want to make an observation: The ability to break a not taken loop backedge is a core property of "sparse conditional" style algorithms. They solve this problem as a special case of a more general data flow problem. I checked, and both SCCP and NewGVN are capable of breaking the backedge on all cases in eval_first_iteration.ll that get broken by the this implementation. Neither handle the "loop guard" cases (which needed isKnownPredicateAt), but I believe both could be extended to handle them -- they have the necessary ingredients to do so efficiently via PredicateInfo.

Assuming that those test cases are actually representative, could it be that we're trying to solve a phase ordering problem by introducing a new optimization? For example, there's this FIXME <https://github.com/llvm/llvm-project/blob/c27e8141b3d1265d2ab1cb951c4330b961fab9ee/llvm/lib/Passes/PassBuilder.cpp#L815-L817> that suggests that SCCP should be performed before rather than after loop optimizations. Maybe just doing that move would already address the original motivation? It would be helpful to have the unreduced motivating test-case, as it's hard to evaluate phase ordering otherwise.


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

https://reviews.llvm.org/D102615