[PATCH] D39340: Modifying reassociate for improved CSE

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: include/llvm/Transforms/Scalar/Reassociate.h:76
+
+  unsigned Pass;
+  static const unsigned NumBinaryOps =
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D39340