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

[Jingyue Wu]

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:22

----------------
// We cannot eliminate the partially common subexpression getelementptr [10 x %struct]* %ptr, i64 %i

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:720
@@ +719,3 @@
+      StructType *StTy = cast<StructType>(*GTI);
+      int64_t Field = cast<ConstantInt>(GEP->getOperand(I))->getSExtValue();
+      // Skip field 0 as the offset is always 0.
----------------
Not critical, but I slightly prefer getZExtValue() because a field index is non-negative and getElementOffset takes unsigned. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:853
@@ +852,3 @@
+  // Remove the constant offset in each sequential index. The resultant GEP
+  // computes the variadic base. Notice that we always don't remove a structure
+  // index. If diable LowerGEP, a index is not accumulated and we still use the
----------------
// we always don't => we don't remove struct field indices here

"always don't" is not the right grammar. Besides, you do extract field indices later, so I'd weaken the word "never". 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:854
@@ +853,3 @@
+  // computes the variadic base. Notice that we always don't remove a structure
+  // index. If diable LowerGEP, a index is not accumulated and we still use the
+  // old one. If enable LowerGEP, a structure index is always accumulated in the
----------------
diable => LowerGEP is disabled

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:855
@@ +854,3 @@
+  // index. If diable LowerGEP, a index is not accumulated and we still use the
+  // old one. If enable LowerGEP, a structure index is always accumulated in the
+  // constant offset. The following lowerToXXX functions will just create a
----------------
enable LowerGEP => LowerGEP is enabled

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:856
@@ +855,3 @@
+  // old one. If enable LowerGEP, a structure index is always accumulated in the
+  // constant offset. The following lowerToXXX functions will just create a
+  // new instruction for the whole constant offset and won't need a new
----------------
I wrote lowerToXXX in my reviews just for simplicity. However, in the source code, we should still use the full names. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:184
@@ -130,3 +183,3 @@
   /// Looks for a constant offset without extracting it. The meaning of the
   /// arguments and the return value are the same as Extract.
   static int64_t Find(Value *Idx, const DataLayout *DL, GetElementPtrInst *GEP);
----------------
jingyue wrote:
> You may want to say Find looks for constant offsets in both array indices and field indices. 
> 
> Because the signature of Extract is changed, you need to remove the comment "The meaning of the arguments and the return value ..." and explicitly explain what its arguments and return value mean. 
I don't see the first part of this comment addressed. I'd like to emphasize "both array indices and field indices". 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:721
@@ +720,3 @@
+  for (unsigned I = 1, E = GEP->getNumOperands(); I != E; ++I, ++GTI) {
+    if (!isa<ConstantInt>(GEP->getOperand(I))) {
+      Value *Idx = GEP->getOperand(I);
----------------
HaoLiu wrote:
> jingyue wrote:
> > Why checking whether it's a constant? Shouldn't you check whether it is an array index? My understanding is, the offsets of structure indices (which are guaranteed to be constant) and the constant offsets of array indices are already counted in accumulativeByteOffsets, and now you only need to rebuild the variadic part of the array indices. 
> Yes, you are right. I'll add comments to explain this.
I wasn't just proposing a comment change. I meant the source code should read something like
```
if (isa<SequentialType>(GTI))
```
You are building new GEPs/arithmetics for the remainder of the array indices, and checking whether the indexed type is a SequentialType matches your intention much better. 

I also believe checking for SequentialType instead of ConstantInt solves your previous question on handling "a = 1 + 2". I'll go back to that later. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:827
@@ +826,3 @@
+  // splitting probably won't be beneficial.
+  if (!LowerGEP) {
+    TargetTransformInfo &TTI = getAnalysis<TargetTransformInfo>();
----------------
HaoLiu wrote:
> jingyue wrote:
> > Why do we need to check LowerGEP here? My understanding is, even when LowerGEP is set, +AccumulativeByteOffset will still become part of the lowered GEPs or arithmetics (see how transformToSimplerGEPs create a GEP with offset AccumulativeByteOffset, and how transformToArithmetics create an add with offset AccumulativeByteOffset). Shouldn't we still check whether this offset can be nicely folded into an addressing mode? 
> I think even a pointer plus a constant index can not be matched in an addressing mode, it is worth to lower a GEP with multiple indices. Because we can do optimizations on the variable indices. Also if the constant index is extracted from multiple indices, the result looks better than the original GEP.
> If I remove such check, some test cases will fail. So it is kept when disable LowerGEP.
That makes some sense. Out of curiosity, which test would fail? 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:889
@@ +888,3 @@
+
+void SeparateConstOffsetFromGEP::transformToSimplerGEPs(
+    GetElementPtrInst *GEP, int64_t AccumulativeByteOffset) {
----------------
jingyue wrote:
> HaoLiu wrote:
> > jingyue wrote:
> > > Can we merge the two transformToXXX functions? The logics are pretty similar. 
> > I think we'd better not do merge. They logic look similar, but they do quite different things. If we want to merge them, we need to add checks like "if (TransformToSimplerGEP)" in many places (From the first for loop logic to the function end). That would be a quite hard work for other people to understand the code.
> The more I look at these two functions, the more strongly I believe we should merge them. Especially, after you get rid of ResultIndices, you probably only need to check LowerGEP once in the merged function. No? 
I don't see this comment addressed.

http://reviews.llvm.org/D5864