[PATCH] D147457: [Reassociation] Only form CSE expressions for local operands

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet created this revision.
qcolombet added reviewers: spatel, wristow, lebedev.ri.
Herald added subscribers: StephenFan, asbirlea, hiraditya.
Herald added a project: All.
qcolombet requested review of this revision.
Herald added a project: LLVM.

1. TL;DR #

This patch constrains how much freedom the heuristic that tries to from CSE
expressions has. The added constrain is that the CSE-able expressions must be
within the same basic block as the expressions they get moved before.

1. Details #

The reassociation pass currently tweaks the rewrite of the final expression
towards surfacing pairs of operands that would be CSE-able.

This heuristic applies after the regular ordering of the expression.
The regular ordering uses the program structure to choose in which order each
subexpression is materialized. That order follows the topological order.

Now, to expose more CSE opportunities, this heurisitc effectively bypasses the
previous ordering normally defined by the program and pushes up sub-expressions
that are arbitrary deep in the CFG.
E.g., let's say the program order (top to bottom) gives `((a*b)*c)*d)*e` and
`b*e` appears the most in the program. The expression will be reordered in
`(((b*e)*a)*c)*d`

This reordering implies that all the sub expressions (in this example `xx*a`,
then `yy*c`, etc.) will need to appear after the CSE-able expression.

This may over-constrain where the (sub) expressions may live and in particular
it may create loop-dependent expressions.

This patch only allows to move expressions up the expression chain when the
related values are definied in the same basic block as the ones they
"push-down".

This constrain is far for being perfect but at least it avoids accidentally
creating loop dependent variables.

If we really want to expose CSE-able expressions in a proper way, we would need
a profitability metric and also make the decision globally as opposed to one
chain at a time.

I've put the new constrain behind an option to make comparing the old and new
versions easy. However, I believe that even if we find cases where the old
version performs better it is probably by accident. What I am aiming for with
this change is more predictability, then we can improve if need be.

This fixes www.llvm.org/PR61458

@dsanders Could you try to see if this patch keeps the benefits than Fiona had with b8a330c42ab?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147457

Files:
  llvm/lib/Transforms/Scalar/Reassociate.cpp
  llvm/test/Transforms/Reassociate/defeat-licm.ll
  llvm/test/Transforms/Reassociate/local-cse.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147457.510549.patch
Type: text/x-patch
Size: 16394 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230403/f1d820f1/attachment.bin>