[PATCH] [AArch64] Improve and enable the SeparateConstOffsetFromGEP for AArch64 backend.

[Jingyue Wu]

LGTM with some minor fixes. You may want to double check with ARM folks on performance. I remember Gerolf said he couldn't get the same performance numbers as you did. 

Thanks again for all the hard work!

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:148
@@ +147,3 @@
+//
+// Lower GEPs can also benefit other passes such as LICM and CGP.
+// LICM (Loop Invariant Code Motion) can not hoist/sink a GEP of multiple
----------------
Lower => Lowering

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:150
@@ +149,3 @@
+// LICM (Loop Invariant Code Motion) can not hoist/sink a GEP of multiple
+// indices if one of the index is variant. If lower such GEP into invariant
+// parts and variant parts, LICM can hoist/sink those invariant parts.
----------------
lower => we lower

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:154
@@ +153,3 @@
+// target's addressing modes. A GEP with multiple indices may not match and will
+// not be sunk. If lower such GEP into smaller parts, CGP may sink some of them.
+// So we end up with better addressing mode.
----------------
lower => we lower

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:155
@@ +154,3 @@
+// not be sunk. If lower such GEP into smaller parts, CGP may sink some of them.
+// So we end up with better addressing mode.
+//
----------------
better => a better

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:333
@@ -253,5 +332,3 @@
   bool splitGEP(GetElementPtrInst *GEP);
-  /// Finds the constant offset within each index, and accumulates them. This
-  /// function only inspects the GEP without changing it. The output
-  /// NeedsExtraction indicates whether we can extract a non-zero constant
-  /// offset from any index.
+  /// Lower a GEP with multiple indices into multiple GEPs with single index.
+  /// \p GEP                    The given GEP. It has been extracted a constant
----------------
single index => a single index

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:334
@@ +333,3 @@
+  /// Lower a GEP with multiple indices into multiple GEPs with single index.
+  /// \p GEP                    The given GEP. It has been extracted a constant
+  ///                           offset.
----------------
The grammar of the sentence "It has been extracted a constant offset" is incorrect. 

```
/// Lower a GEP with multiple indices into multiple GEPs with a single index.
/// Function splitGEP already split the original GEP into a variadic part and a
/// constant offset (i.e., AccumulativeByteOffset). This function lowers the
/// variadic part into a set of GEPs with a single index and applies
/// AccumulativeByteOffset to it. 
/// GEP The variadic part of the original GEP
/// AccumulativeByteOffset ... 
```
I'd even like to change the name of the argument GEP to Variadic, because GEP appears so many times throughout this file and usually refers to the original GEP. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:343
@@ +342,3 @@
+  /// \p AccumulativeByteOffset The extracted constant offset.
+  void lowerToArithmetics(GetElementPtrInst *GEP,
+                          int64_t AccumulativeByteOffset);
----------------
ditto

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:348
@@ -258,1 +347,3 @@
+  /// types, otherwise it only finds in sequential indices. The output
+  /// NeedsExtraction indicates whether we can find a non-zero constant offset.
   int64_t accumulateByteOffset(GetElementPtrInst *GEP, bool &NeedsExtraction);
----------------
we can find => we successfully find

My original comment wasn't clear enough, and I think "we successfully find" is better. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:370
@@ +369,3 @@
+  /// Whether to lower a GEP with multiple indices into arithmetic operations or
+  /// multiple GEPs with single index.
+  bool LowerGEP;
----------------
single index => a single index

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:850
@@ +849,3 @@
+  // the addressing mode, we can still do optimizations to other lowered parts
+  // of variable indices. So we don't such check.
+  if (!LowerGEP) {
----------------
The grammar of "So we don't such check" is wrong. => Therefore, we don't check for addressing modes in that case. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:865
@@ +864,3 @@
+  // one. If LowerGEP is enabled, a structure index is accumulated in the
+  // constant offset. The following lowerToSingleIndexGEPs and
+  // lowerToArithmetics will handle the constant offset and won't need a new
----------------
The following xxx and yyy => lowerToSingleIndexGEPs or lowerToArithmetics will later

http://reviews.llvm.org/D5864