[PATCH] Simplify n-ary adds by reassociation

[Jingyue Wu]

Thanks Andrew and Sanjoy for the review! All comments are addressed inline.


================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:73
@@ +72,3 @@
+using namespace llvm;
+using namespace PatternMatch;
+
----------------
sanjoy wrote:
> Since you're using `PatternMatch` in only one function, I think the namespace should be opened in just that scope.
I have a pending patch based on this, and that patch will use `PatternMatch` too. So I'll leave the code as is for now. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:140
@@ +139,3 @@
+  SeenExprs.clear();
+  for (auto node = GraphTraits<DominatorTree *>::nodes_begin(DT);
+       node != GraphTraits<DominatorTree *>::nodes_end(DT); ++node) {
----------------
sanjoy wrote:
> I think LLVM convention is `Node`.  Also, I'd suggest using `auto *Node` to make it obvious that we have a pointer here (very minor).
Changing `auto` to `auto *` doesn't compile. I guess it's because `nodes_begin` returns an iterator and the compiler is unable to deduce `auto *` from the iterator type. Additionally, even if the code compiled, `++Node` would be problematic. 

================
Comment at: lib/Transforms/Scalar/NaryReassociate.cpp:199
@@ +198,3 @@
+       ++LHS, ++NumIterations) {
+    if (DT->dominates(*LHS, I)) {
+      Instruction *NewI = BinaryOperator::CreateAdd(*LHS, RHS, "", I);
----------------
sanjoy wrote:
> Is it possible to switch to a traversal and expression gathering scheme that will always visit defs before uses?  Reverse post-order maybe?  I think that way we can avoid the `dominates` query (which can be expensive) and just turn it into an `assert`; and just use the earliest (latest?) element in `LHSCandidates`.
We traversed the instructions in the pre-order of the dominator tree, so defs are already always visited before uses. However, this doesn't avoid checking `dominates(*LHS, I)` because `*LHS` being visited before `I` does not mean `*LHS` dominating `I`. 

Anyhow, I think there is a way of avoiding the dominance check, and I'm happy to put it in the next patch. Whenever we backtrack from a basic block during DFS'ing the dominator tree, we remove all SCEVs defined in this basic block from `SeenExprs`. This can ensure that, at any time, all the instructions in `SeenExprs` dominate the basic block we are exploring. 

For example, suppose the dominator tree of this function looks like:
```
B1 -> B2
 |
 V
B3 -> B4
 |
 V
B5
```
Then, we traverse the tree in this order: +B1 +B2 -B2 +B3 +B4 -B4 +B5 -B5 -B3 -B1. + means entering a node, and - means exiting a node. 

Let's pick a random basic block say B4. Before entering B4, we added B1, B2, and B3 to `SeenExprs` and removed B2 from it, so `SeenExprs` contains instructions in B1 and B3 which must dominate B4.

http://reviews.llvm.org/D8950

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