[PATCH] D40049: [PATCH] Global reassociation for improved CSE

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added a comment.

Tests look good.



================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2190
   }
 
+  if (ReassociateStep == 1 && Ops.size() > 2 &&
----------------
Please provide some high level comments here explaining what is happening like all the other steps in the same function.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2197
+    unsigned Idx = I->getOpcode() - Instruction::BinaryOpsBegin;
+    for (unsigned i = 0; i < Ops.size() - 1; ++i)
+      for (unsigned j = i + 1; j < Ops.size(); ++j) {
----------------
Style nitpick: Might be clear to read of both for loops have brackets.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2198
+    for (unsigned i = 0; i < Ops.size() - 1; ++i)
+      for (unsigned j = i + 1; j < Ops.size(); ++j) {
+        unsigned Score = 0;
----------------
Could there be a way to do this without a nesting loop? If not ignore this.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2265
+      unsigned BinaryIdx = I.getOpcode() - Instruction::BinaryOpsBegin;
+      SmallSet<std::pair<Value *, Value*>, 32> Visited;
+      for (unsigned i = 0; i < Ops.size() - 1; ++i) {
----------------
This next block of nested for loops looks similar to the above best pair code (that is also part of this patch). Could you possibly refactor it to reuse this same pattern in a helper?


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2296
   // Traverse the same blocks that was analysed by BuildRankMap.
-  for (BasicBlock *BI : RPOT) {
-    assert(RankMap.count(&*BI) && "BB should be ranked.");
-    // Optimize every instruction in the basic block.
-    for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;)
-      if (isInstructionTriviallyDead(&*II)) {
-        EraseInst(&*II++);
-      } else {
-        OptimizeInst(&*II);
-        assert(II->getParent() == &*BI && "Moved to a different block!");
-        ++II;
+  for (ReassociateStep = 0; ReassociateStep < 2; ++ReassociateStep) {
+    for (BasicBlock *BI : RPOT) {
----------------
Add comment explaining the reason for having 2 reassociate steps. Also consider assining 2 to a value, call it const unsigned MaxReassociatreSteps = 2; And put the comment with the assignment.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:2334
     }
-
-    // Now that we have removed dead instructions, we can reoptimize the
-    // remaining instructions.
-    while (!RedoInsts.empty()) {
-      Instruction *I = RedoInsts.pop_back_val();
-      if (isInstructionTriviallyDead(I))
-        EraseInst(I);
-      else
-        OptimizeInst(I);
-    }
+    if (ReassociateStep == 0)
+      BuildPairMap(RPOT);
----------------
Could we populate the pair map prior to the first iteration? If so, Move this before the loop starts.


Repository:
  rL LLVM

https://reviews.llvm.org/D40049