[PATCH] D39340: Modifying reassociate for improved CSE

[escha via Phabricator via llvm-commits]

escha added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/Reassociate.h:76
+
+  unsigned Pass;
+  static const unsigned NumBinaryOps =
----------------
fhahn wrote:
> arsenm wrote:
> > I find the use of this somewhat confusing. It seems to really be a loop counter stuck in the class to pass it to the other functions. Is it particularly inconvenient to pass this in to the functions where it's needed? Also a better name might be ReassociateStep or Level (possibly with an enum for the steps)? Pass is pretty overloaded (plus lldb doesn't understand what to do with this since there is also a class named Pass)
> I am probably missing something, but would it be possible to collect the linear chains as a pre-processing step and then do the full reassociate only once? You might have to move out the code to linearize binary ops, but I think it would be a more lightweight and clearer solution.
Mmm, I'm not sure. The problem is that the chains can *change* as a result of reassociation, which can rewrite instructions. e.g. changing use-counts can change the chains.

You're probably right; the main reason I didn't do that was that I wanted to make minimally invasive changes because I really don't feel I understand the pass deeply enough to restructure it like that safely.


================
Comment at: include/llvm/Transforms/Scalar/Reassociate.h:76
+
+  unsigned Pass;
+  static const unsigned NumBinaryOps =
----------------
escha wrote:
> fhahn wrote:
> > arsenm wrote:
> > > I find the use of this somewhat confusing. It seems to really be a loop counter stuck in the class to pass it to the other functions. Is it particularly inconvenient to pass this in to the functions where it's needed? Also a better name might be ReassociateStep or Level (possibly with an enum for the steps)? Pass is pretty overloaded (plus lldb doesn't understand what to do with this since there is also a class named Pass)
> > I am probably missing something, but would it be possible to collect the linear chains as a pre-processing step and then do the full reassociate only once? You might have to move out the code to linearize binary ops, but I think it would be a more lightweight and clearer solution.
> Mmm, I'm not sure. The problem is that the chains can *change* as a result of reassociation, which can rewrite instructions. e.g. changing use-counts can change the chains.
> 
> You're probably right; the main reason I didn't do that was that I wanted to make minimally invasive changes because I really don't feel I understand the pass deeply enough to restructure it like that safely.
And to the name, absolutely, will change on next iteration regardless.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2217
+
+        unsigned MaxRank = std::max(Ops[i].Rank, Ops[j].Rank);
+        if (Score > Max || (Score == Max && MaxRank < BestRank)) {
----------------
hiraditya wrote:
> Does this ensure that the original order of evaluation is emitted when there is no preferred order?
I *think* it should? Not 100% sure. It currently doesn't reorder if Score never exceeds 1 but I don't fully trust this code. There's probably cases where it's "wrong" (as in, doing something unnecessary/suboptimal with order, not incorrect code).


Repository:
  rL LLVM

https://reviews.llvm.org/D39340