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

[Ahmed Bougacha via Phabricator via llvm-commits]

ab accepted this revision.
ab added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:2442
+          // Do as if it lives in the entry block.
+          SeenBB = &I->getParent()->getParent()->getEntryBlock();
+        } else {
----------------
qcolombet wrote:
> ab wrote:
> > Why the entry block, would it make sense to continue here and skip the value in this case?
> Good point, I should probably add a comment here.
> The reason I do that is because while we can skip the first value like that, we can't skip them if there are more than one that fall into this category.
> 
> To step back a little bit, these values are either arguments or global variables. Forget about global variables, there're not interesting for reassociation.
> 
> By default the order of the values not having a block is going to be very early in the chain (left most expression computed first):
> `reassoc_expr = arg1 op arg2 op arg3 op ... op real_val1 op ...`
> 
> Therefore the beginning of the chain can be scheduled freely and the result will feed the rest of the chain.
> 
> Now, if we skip this value, we're effectively saying that it is okay to put them wherever in the chain. This means that we're going to potentially constraint where the sub expressions for these arguments will live and in particular we may be pushing their computation in loops.
> E.g.,
> `reassoc_expr = real_val1 op arg1 op arg2 op arg3 op ... op real_val2 op ...`
> If `real_val1` and `real_val2` are in a loop, now all the intermediate computations must be in a loop as well.
> 
> We could be cleverer about anchoring these expression, but I thought it was not worth the complexity.
Makes sense, thanks for the explanation!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147457