[PATCH] [NaryReassoc] reassociate GEP for CSE

[Bjarke Hammersholt Roune]

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

================
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) {
----------------
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?

================
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);
----------------
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.

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:237
@@ -183,4 +236,3 @@
     for (auto I = BB->begin(); I != BB->end(); ++I) {
-      // Skip vector types which are not SCEVable.
-      if (I->getOpcode() == Instruction::Add && !I->getType()->isVectorTy()) {
-        if (Instruction *NewI = tryReassociateAdd(I)) {
+      // Skip non-SCEVable types.
+      if (!SE->isSCEVable(I->getType()))
----------------
Previously, the comment about SCEVable was helpful as it was not so clear that that was what isVectorTy was about. I think it's clear that isSCEVable checks if the type is SCEVable, so then the comment could be removed.

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

  if (!SE->isSCEVable(I->getType()) || !isPotentiallyNaryReassociable(I)) {

================
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.
----------------
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.

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:264
@@ +263,3 @@
+  default:
+    llvm_unreachable("should be filtered out by isPotentiallyNaryReassociable");
+  }
----------------
What's the motivation behind doing it this way instead of returning null here and then not needing isPotentiallyNaryReassociable?

================
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);
----------------
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?

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

================
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
----------------
"in the pre-order" -> "in pre-order" or perhaps "Because we do a pre-order traversal of the dominator tree"

http://reviews.llvm.org/D9802

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