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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:135
+nonLeafNodesNeededForTree(unsigned LeafNo,
+                          SmallVector<unsigned, 8> &DepthToNodesNum) {
+  unsigned TotalRemainingNodes = LeafNo - 1;
----------------
Consider passing `SmallVectorImpl<unsigned>` to avoid adding noise in form of size.


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:138
+  for (unsigned i = 0; TotalRemainingNodes > 0; ++i) {
+    unsigned MaxNodesForDepth = 1 << i;
+    if (TotalRemainingNodes <= MaxNodesForDepth)
----------------
I think you may want to suffix that one with `U` https://godbolt.org/g/FDQPax
Or maybe there is some helper function to do that?


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:154-155
+static void updateTree(
+    const SmallVector<SmallVector<BinaryOperator *, 8>, 8> &TreeNonLeaves,
+    const SmallVector<Value *, 8> &Leaves) {
+  unsigned i = 0; // iterator over the leaves
----------------
Consider passing `SmallVectorImpl<>` to avoid adding noise in form of size.


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:209
+  SmallVector<Value *, 8> Ops;
+  std::stack<BinaryOperator *> S;
+  S.push(BO);
----------------
You could do `std::stack<BinaryOperator *, SmallVector<BinaryOperator *, 4>>` to still make use of SSO


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:239
+  BasicBlock *BB = I->getParent();
+  NonLeaves.push_back(SmallVector<BinaryOperator *, 8>{I});
+  for (unsigned Depth = 1;; ++Depth) {
----------------
Do you actually need to specify the type here?


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:248-249
+    for (BinaryOperator *PrevDepthNode : PrevDepthNodes) {
+      for (unsigned i = 0; i < 2; ++i) {
+        Value *Operand = PrevDepthNode->getOperand(i);
+        BinaryOperator *OperandBO = dyn_cast<BinaryOperator>(Operand);
----------------
I *think* this is the only use of `i`?
wouldn't
```
for (Value *Operand : PrevDepthNode->operands())
```
work?


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:263
+          if (CurrDepthNodes == nullptr) {
+            NonLeaves.push_back(SmallVector<BinaryOperator *, 8>());
+            CurrDepthNodes = &NonLeaves[Depth];
----------------
Wouldn't just `NonLeaves.push_back({});` work?


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:283
+
+  std::map<LeafClass, SmallVector<Value *, 8>> LeavesSets;
+  // Dividing the leaves into sets according to their LeafClass.
----------------
Humm, have you considered [[ https://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h | `llvm::DenseMap<>` ]]?


================
Comment at: lib/Transforms/Scalar/WeakReassociate.cpp:285
+  // Dividing the leaves into sets according to their LeafClass.
+  for (unsigned i = 0, e = Leaves.size(); i != e; ++i) {
+    LeafClass OpClass;
----------------
Couldn't this be range-for loop?


Repository:
  rL LLVM

https://reviews.llvm.org/D41574