[PATCH] D41574: [Transforms] Adding a WeakReassociate pass

[Omer Paparo Bivas via Phabricator via llvm-commits]

opaparo marked 9 inline comments as done.
opaparo added a comment.

In https://reviews.llvm.org/D41574#1110465, @mzolotukhin wrote:

> What are benchmarks results with this pass (compile-time/performance)? Do we really need it at all optlevels, or can we only include it at -O3?


I've got some minor compile time regressions on my benchmarks, but nothing significant.
Judging by this ongoing discussion <https://groups.google.com/forum/#!topic/llvm-dev/kOgf62tmLlY>, this pass can potentially become a complimentary pass to InstCombine. If that will be the case, it should be active in all opt levels.



================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:103
+    return false;
+  return LHS.Op1->getValue().slt(RHS.Op1->getValue());
+}
----------------
lebedev.ri wrote:
> It is hopefully nothing, but what if they compare equal?
> I'm basically thinking about the `llvm::sort()` being intentionally unstable kind of things.
If they compare equal then they are the same LeafClass, and should correspond to the same cell in the map.
I've indeed had some trouble with stable sorting in the past, and that's exactly why the Op1s of the two sides are compared by value and not by pointer (which may lead to different outcomes on different platforms or even consecutive runs on the same machine).


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:209
+  SmallVector<Value *, 8> Ops;
+  std::stack<BinaryOperator *> S;
+  S.push(BO);
----------------
lebedev.ri wrote:
> You could do `std::stack<BinaryOperator *, SmallVector<BinaryOperator *, 4>>` to still make use of SSO
Done, but just so I'm sure - what does SSO stand for?


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:283
+
+  std::map<LeafClass, SmallVector<Value *, 8>> LeavesSets;
+  // Dividing the leaves into sets according to their LeafClass.
----------------
lebedev.ri wrote:
> Humm, have you considered [[ https://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h | `llvm::DenseMap<>` ]]?
I've tried it.
The thing is that llvm::DenseMap does not guarantee the order of the elements, unlike std::map. It of course guarantees that two different key types will be mapped to two different cells in the map, but when iterating over it (as I do later on) you can get any order. When experimenting with it, I got two different iterating orders in two different runs, which resulted in different code in each run.
Technically I could use llvm::sort after I'm done filling the map, but I find it strange to do that instead of using a data structure that already keeps the elements sorted.


Repository:
  rL LLVM

https://reviews.llvm.org/D41574