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

[Jingyue Wu]

Regarding the correctness issue with struct field indices, I think the solutions you proposed make sense. Look forward to your fix in the next diff. We should choose which solution to go with depending on useAA. 

The logic looks good in general except the field index issue. My inline comments are mostly on refactoring.

================
Comment at: lib/Target/AArch64/AArch64TargetMachine.cpp:208
@@ +207,3 @@
+    addPass(createEarlyCSEPass());
+    // Do loop invariant code motion in case any of the address calculations are
+    // actually loop invariant.
----------------
I'd explain why running GEP reassociation can expose more opportunities for LICM. I guess only the split part (splitting a GEP with multiple indices into a set of smaller GEPs) buys you such opportunities. Right? 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:82
@@ -81,1 +81,3 @@
 //
+//
+// An improvement is to simplify complex GEPs to simpler forms:
----------------
Remove superfluous blank lines. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:83
@@ +82,3 @@
+//
+// An improvement is to simplify complex GEPs to simpler forms:
+// Even we can extract constants from GEPs, sometimes it is still unable to do
----------------
// Another improvement enabled by the SimplifyGEP flag is ... 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:93
@@ +92,3 @@
+//     basic blocks, CodeGen will not be able to tell if there are common
+//     subexpressions between them.
+//
----------------
I don't understand reason 2. Can you explain more? Are you saying, given a[i][j1] and a[i][j2], you can reuse the common part &a[i]? 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:365
@@ -277,1 +364,3 @@
+  /// operations or several simpler GEPs).
+  bool SimplifyGEP;
 };
----------------
What would be a good name? You are not really *simplifying* but to some extend *complicating* GEPs :)

Does LowerGEP sound good to you? You are *lowering* a GEP into multiple uglygeps (btw, uglygep is a known concept in LLVM) or arithmetic operations which are closer to machine code. 

Make sure the comments are consistent after renaming. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:857
@@ -765,1 +856,3 @@
 
+int64_t SeparateConstOffsetFromGEP::accumulateByteOffsetForSeqAndStruct(
+    GetElementPtrInst *GEP, bool &NeedsExtraction) {
----------------
Similarly, this function should be merged into accumulateByteOffset. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:889
@@ +888,3 @@
+
+void SeparateConstOffsetFromGEP::transformToSimplerGEPs(
+    GetElementPtrInst *GEP, int64_t AccumulativeByteOffset) {
----------------
Can we merge the two transformToXXX functions? The logics are pretty similar. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:988
@@ +987,3 @@
+
+bool SeparateConstOffsetFromGEP::extractConstantAndSimplifyGEP(
+    GetElementPtrInst *GEP) {
----------------
Looks quite similar to splitGEP. Can we massage this into splitGEP, and guard the part that lowers GEP with if (SimplifyGEP)? 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1016
@@ +1015,3 @@
+          ConstantOffsetExtractor::Extract(GEP->getOperand(I), DL, GEP);
+      if (NewIdx != nullptr) {
+        // Watch out for the index like a = 1 + 2. If such index is analyzed,
----------------
OK, now I see what you're doing here. According to our discussion in D5863, this looks like a temporary work around for the CFGSimplification issue, and will eventually go away. I don't think it's a good idea to hack this pass to work around that issue. I'd rather run instsimplify before SeparateConstOffset in the AArch64 backend, and add a FIXME there saying once the CFGSimplification issue is fixed we needn't run instsimplify any more. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1050
@@ +1049,3 @@
+  // ptrtoint + arithmetic + inttoptr form.
+  if (SimplifyGEP) {
+    for (auto &B : F) {
----------------
After extractConstantAndSimplifyGEP is merged into splitGEP, the code here can be simplified.

http://reviews.llvm.org/D5864