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

[Jingyue Wu]

My general concern is the part extracting constant struct field index seems wrong. Unlike arrays where every elements share the same type, structure fields can have different types. Therefore, changing a field index may change the intermediate type in a pointer arithmetic, causing incorrect IR. 

For example, consider
```
struct S {
  int a[4]; // sizeof(int) = 4
  double b[8]; // sizeof(double) = 8
};

%s = alloca struct %S
%p = getelementptr %s, 0, 1, i ; &s.b[i] = s + 4*sizeof(int) + i*sizeof(double)
```
If you extract the 1, the gep becomes:
```
%p = getelementptr %s, 0, 0, i; &s.a[i] = s + i*sizeof(int)
```
and the constant offset is 4 * sizeof(int) = 16. 

As you probably see already,
```
s + 4*sizeof(int) + i*sizeof(double) != s + 4*sizeof(int) + i*sizeof(int)
```

Does this make sense to you? What is a good way to fix this? 

I also have a bunch of nit picks inlined.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:88
@@ +87,3 @@
+// fully CSE for complex GEPs. There are two reasons:
+// (1) The above algorithm can not extract constants in structure type indices.
+//     Actually the indices of structure types in GEP are always constants which
----------------
Nice explanation! However, in the final version, you may want to merge these improvements into the old comments. You can mention the improvements in the summary of the revision. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:185
@@ -122,3 +184,3 @@
   /// new index representing the remainder (equal to the original index minus
   /// the constant offset).
   /// \p Idx    The given GEP index
----------------
//, or null if we cannot extract a constant offset. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:319
@@ +318,3 @@
+  /// it into simpler forms of several simpler GEPs or an arithmetic form
+  /// (depends on wheter the target use alias analysis in codegen).
+  bool extractConstantAndSimplifyGEP(GetElementPtrInst *GEP);
----------------
typo:
the the given => the given
a constant offsets => a constant offset
wheter => whether
depends on => depending on
use => uses
several simpler GEPs or arithmetic operations

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:331
@@ +330,3 @@
+  /// \p AccumulativeByteOffset The extracted constant offset.
+  void transformToAdds(GetElementPtrInst *GEP, int64_t AccumulativeByteOffset);
+  /// Finds the constant offset within each index for sequential types, and
----------------
a better name would be transformToArithmetics because you are using not only adds but also muls

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:364
@@ -277,1 +363,3 @@
+  const TargetMachine *TM;
+  bool SimplifyGEP;
 };
----------------
Add a comment on what SimplifyGEP means. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:642
@@ -552,2 +641,3 @@
                      GEP->isInBounds());
+  Value *NewIdx = nullptr;
   if (ConstantOffset != 0) {
----------------
  If (ConstantOffset == 0)
    return NewIdx;
  // ...
  return Extractor.rebuild...

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:878
@@ +877,3 @@
+      StructType *StTy = cast<StructType>(*GTI);
+      unsigned Field = cast<ConstantInt>(GEP->getOperand(I))->getZExtValue();
+      // Skip field 0 as the offset is always 0.
----------------
getZExtValue() returns uint64_t. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1019
@@ +1018,3 @@
+      if (NewIdx != nullptr) {
+        // For the index like a = 1 + 2, which we can only find 2 and return 1
+        // as the NewIdx. We also check and accumulate such constant index.
----------------
The comment here is confusing. It doesn't seem to explain what the following code does. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1021
@@ +1020,3 @@
+        // as the NewIdx. We also check and accumulate such constant index.
+        if (ConstantInt *OtherConst = dyn_cast<ConstantInt>(NewIdx)) {
+          if (!OtherConst->isZero()) {
----------------
Why name it *Other*Const?

http://reviews.llvm.org/D5864