[PATCH] D151616: [Transforms][Reassociate] "Reassociate expressions" pass optimizations not always profitable

[Paul Osmialowski via Phabricator via llvm-commits]

pawosm01 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:1742
+          if ((hasFactor(Operand, MaxOccVal)) && (!isInOneLoop(LI, Operand, L)))
+            return nullptr; // Transformation not profitable, giving up.
+        }
----------------
qcolombet wrote:
> TL;DR I think the model should be more general than that but as it is, it is probably a good proxy for not shooting ourselves in the foot.
> 
> The problem is not pulling something into a loop, the problem is replacing X something with 1 * frequency-of-the-target-basic-block something.
> 
> I.e., let's imagine we have:
> v1 = s1  * delta
> v2 = s2 * delta
> ...
> vN = sN *delta
> for () {
>   v1 + ... + vN // with loop dependent stuff
> }
> 
> Transforming this into:
> for () {
>   (s1 + ... + sN) * delta
> }
> 
> Could actually be beneficial as long as the loop count is smaller than N.
> 
> Similarly, something like:
> for () {
>   if (unlikely)
>    v1 + ... + vN
> }
> 
> Is even more likely to be beneficial since the expected frequency of the basic block may be smaller than the source basic block.
> Transforming this into: Could actually be beneficial as long as the loop count is smaller than N.

Yes, I was afraid that with this change I'm cutting off some of the beneficial transformations, and making the logic of this pass more complicated than necessary.

Therefore I tend to agree with Florian, that this problem could be addressed in LICM instead. In the end, it's not a wrong thing for a transformation pass to undo whatever previous pass(es) did when there is a new knowledge available (and here, the new knowledge is that we are in a loop). I'm very close to giving up this commit and work on extending `hoistArithmetics()` (of LICM) with the ability to spot this code pattern. As I mentioned in the other comment, currently LICM can't figure it out even if it wasn't caused by Reassociate pass, namely a simple C code that I specifically crafted to replicate this problem was enough to show that GCC produces faster executable than LLVM. But there's a problem with LICM too. This `hoistArithmetics()` function and its surrounding is getting more and more complicated already. And I assume any attempt to extend it with ability to deal with the situation discussed here, will create a lot of code bloat. But maybe that's something inevitable.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151616