[PATCH] [NaryReassoc] reassociate GEP for CSE

[Jingyue Wu]

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:130
@@ +129,3 @@
+
+  // Reasssociates I for better CSE.
+  Instruction *tryReassociate(Instruction *I);
----------------
broune wrote:
> sss -> ss in reasssociates.
Nice caaatch :)

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:217
@@ +216,3 @@
+// Filter out instruction types that won't be reassociable or become a
+// candidate for other instructions.
+static bool isPotentiallyNaryReassociable(Instruction *I) {
----------------
broune wrote:
> It's unclear to me what reassociable means here. Certainly other operations than add and GEPs can be reassociated in certain cases and I'd imagine that one can usually construct some example where that opens up further optimization opportunities. This seems more like a whitelist of instructions that nary reassociate knows how to deal with?
Yes, you are right. It simply whitelists the instruction types we currently handle. Updated the comments accordingly. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:232
@@ -178,3 +231,3 @@
   // Traverse the dominator tree in the depth-first order. This order makes sure
   // all bases of a candidate are in Candidates when we process it.
   for (auto Node = GraphTraits<DominatorTree *>::nodes_begin(DT);
----------------
broune wrote:
> in the depth-first order -> in depth-first order
> makes sure -> ensures that
> 
> You state more specifically that the traversal is in pre-order in another comment, and I think that that is necessary - it's not enough for the order to be depth-first. So this comment could also say pre-order.
Done. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:240
@@ +239,3 @@
+        continue;
+      if (isPotentiallyNaryReassociable(I)) {
+        if (Instruction *NewI = tryReassociate(I)) {
----------------
broune wrote:
> This could be moved into the previous if as
> 
>   if (!SE->isSCEVable(I->getType()) || !isPotentiallyNaryReassociable(I)) {
Done. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:248
@@ -192,3 +247,3 @@
         }
-        // We should add the rewritten instruction because tryReassociateAdd may
+        // We should add the rewritten instruction because tryReassociate may
         // have invalidated the original one.
----------------
broune wrote:
> Is this explaining why it is that we are not using the old I from above? If so, I'd say that it's not just because of tryReassociate, it's more because we called replaceAllUsesWith and then RecursivelyDeleteTriviallyDeadInstructions above. I'm not sure it requires a comment to explain that we then can't use the old I.
It's tempting to put this statement before the "if tryReassociate" block. I was protecting code against that. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:264
@@ +263,3 @@
+  default:
+    llvm_unreachable("should be filtered out by isPotentiallyNaryReassociable");
+  }
----------------
broune wrote:
> What's the motivation behind doing it this way instead of returning null here and then not needing isPotentiallyNaryReassociable?
If `!isPotentiallyNaryReassociable`, we don't need to add `I` to `SeenExprs`. I could merge `SeenExprs[SE->getSCEV(I)].push_back(I)` to `tryReassociate`; then `tryReassociate` should probably be renamed to `tryReassociateAndAddCandidate`? 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:341
@@ +340,3 @@
+    if (requiresSignExtension(IndexToSplit, GEP) && !AO->hasNoSignedWrap())
+      return nullptr;
+    Value *LHS = AO->getOperand(0), *RHS = AO->getOperand(1);
----------------
broune wrote:
> I was wondering about the case where the index is unsigned so that hasNoUnsignedWrap would be relevant, but it looks like GEP always has signed indices. Is that how it works?
If the indexed is unsigned in the source code, then IR either zero-extends it (which we currently ignore) to pointer size or treats it as signed if it's already pointer size. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:346
@@ +345,3 @@
+      return NewGEP;
+    // Symmetrically, try IndexToSplit = RHS + LHS.
+    if (LHS != RHS) {
----------------
broune wrote:
> I think that one of these comments should say LHS + RHS.
Done. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:474
@@ -243,2 +473,3 @@
+  auto &Candidates = Pos->second;
   // Because we traverse the dominator tree in the pre-order, a
   // candidate that doesn't dominate the current instruction won't dominate any
----------------
broune wrote:
> "in the pre-order" -> "in pre-order" or perhaps "Because we do a pre-order traversal of the dominator tree"
Done.

http://reviews.llvm.org/D9802

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/