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

[Hao Liu]

Hi Jingyue,

I've attached a new patch according to the review comments.
Review please.

Thanks,
-Hao

================
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.
----------------
jingyue wrote:
> Not critical, but I slightly prefer getZExtValue() because a field index is non-negative and getElementOffset takes unsigned. 
Aha, previously I use getZExtValue(). You commented I should change to getSExtValue(). I thought it was not critical, so I modified it. 
Now, It seems I should roll back...
: )

================
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:
> 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". 
I think "both array indices and field indices" means all indices. We don't need to explain this common behavior. It is quite obvious. I think we need comments if we only find in one kind of "array indices" or "field indices", or do something special.
Anyway, the new comments will be like the comments for Extract()

================
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);
----------------
jingyue wrote:
> 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. 
If check SequentialType, we also need to check wheter it is ConstantInt. Then the code will be like:
    if (isa<SequentialType>(GTI))
      if (!isa<ConstantInt>(GEP->getOperand(I))
         ...
      else
         AccumulativeByteOffset += Const
This looks more complicate.

About the "a = 1 + 2", I think handling it when extracting constant in indices is better. Then we don't need to add code in both lowerToXXX functions. Also if we handle it in lowerToXXX functions, the logic for the original splitGEP (I.E. when lowerGEP is diabled) can still not handle this problem.


================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:827
@@ +826,3 @@
+  // splitting probably won't be beneficial.
+  if (!LowerGEP) {
+    TargetTransformInfo &TTI = getAnalysis<TargetTransformInfo>();
----------------
jingyue wrote:
> 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? 
Oh, interesting. I supposed some of your test cases in test/Transforms/SeparateConstOffsetFromGEP/NVPTX may fail.
But I tried removing such check and ran "make check", there is no test failures. So it seems there is no such test cases...

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:889
@@ +888,3 @@
+
+void SeparateConstOffsetFromGEP::transformToSimplerGEPs(
+    GetElementPtrInst *GEP, int64_t AccumulativeByteOffset) {
----------------
jingyue wrote:
> 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. 
I still doubt about your suggestion. The diff 16027 gets ride of ResultIndices. They are still quite different and difficult to combine them.
Lowering to arithmetic operations starts from creating ptrtoint, and then creates ADD/MUL/SHL, and ends with creating ADD and inttoptr. 
Lowering to GEPs starts from creating bitcast, and then creates MUL/SHL/GEPs, and ends with creating GEP and bitcast. 

================
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,
----------------
jingyue wrote:
> HaoLiu wrote:
> > jingyue wrote:
> > > 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. 
> > I just don't think this is a temporary work. We cannot assume there is no situation like "a = 1+2". People also can create a test case of "a = 1 + 2" and feed to this pass. I mean people can call this pass without any other passes. So if we don't accumulate all none zero indices in this place, we'll get incorrect result.
> > 
> > We want to "// Remove the constant offset in each GEP index" and "// Splits this GEP index into a variadic part and a constant offset" in this for loop. So the later part about transformation won't consider about the none zero constant indices, it assumes there is all varaible indices or zero indices. See that I only check "if (!isa<ConstantInt>(GEP->getOperand(I)))". So if we leave an index of "constant 1" behand, it will be ignored and we'll generate incorrect result. That's why I want to make sure we have accumulated all the none zero constant indices.
> I'm confused. Check my comment on the line 
> 
> ```
> if (!isa<ConstantInt>(GEP->getOperand(I)))
> ```
> and if my comment there makes sense I'll come back here. 
I think this is the right place to do such thing. As the comments say, we want to "// Remove the constant offset in each GEP index." If we should leave some constant offset in index.
Also, if we fix such things in the following lowerToXXX functions, the logic for the original splitGEP (I.E. when lowerGEP is diabled) can still not handle this problem.

http://reviews.llvm.org/D5864