[PATCH] [SeparateConstOffsetFromGEP] Fix bugs and improve the current SeparateConstOffsetFromGEP

[Jingyue Wu]

Thanks for working on this! 

First round. 

I'll fix the two correctness bugs separately. After that, please rebase your diff against the new version. 

I am very curious what CFGSimplificationPass does to generate a = 1 + 2. Does a = 1 + 2 appear at the end of -O3? I don't think leaving that clear misoptimization at the end of -O3 is reasonable. 

Jingyue

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:120
@@ -119,4 +119,3 @@
  public:
   /// Extracts a constant offset from the given GEP index. It outputs the
   /// new index representing the remainder (equal to the original index minus
----------------
s/outputs/returns/

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:129
@@ -130,2 +128,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,
----------------
Not the same as Extract any more. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:475
@@ -454,3 +474,3 @@
 Value *
-ConstantOffsetExtractor::distributeExtsAndCloneChain(unsigned ChainIndex) {
+ConstantOffsetExtractor::distributeExtsAndCloneChain(unsigned &ChainIndex) {
   User *U = UserChain[ChainIndex];
----------------
Why do you prefer "unsigned &ChainIndex" to "unsigned ChainIndex"? The latter seems to use more space because unsigned& takes the size of a pointer while unsigned only takes 32 bits. Also, the original logic seems clearer. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:712
@@ -654,8 +711,3 @@
     if (isa<SequentialType>(*GTI)) {
-      Value *NewIdx = nullptr;
-      // Tries to extract a constant offset from this GEP index.
-      int64_t ConstantOffset =
-          ConstantOffsetExtractor::Extract(GEP->getOperand(I), NewIdx, DL, GEP);
-      if (ConstantOffset != 0) {
-        assert(NewIdx != nullptr &&
-               "ConstantOffset != 0 implies NewIdx is set");
+      // Tries to extract constant offsets and build new index.
+      Value *NewIdx =
----------------
// Splits this GEP index into a variadic part and a constant offset, and uses the variadic part as the new index. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:739
@@ -685,1 +738,3 @@
   GEP->setIsInBounds(false);
+  // No need to create another GEP as the accumulative byte offset is 0.
+  if (AccumulativeByteOffset == 0)
----------------
Not quite true. The sum of offsets being 0 doesn't mean every offset is zero. As long as there's a non-zero offset, it's worth creating a new GEP. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:774
@@ -717,3 +773,3 @@
 
-  uint64_t ElementTypeSizeOfGEP =
-      DL->getTypeAllocSize(GEP->getType()->getElementType());
+  // Cast to int64_t as it used with int64_t.
+  int64_t ElementTypeSizeOfGEP = static_cast<int64_t>(
----------------
Nice find! I like this bug :)

================
Comment at: test/Transforms/SeparateConstOffsetFromGEP/AArch64/split-gep.ll:4
@@ +3,3 @@
+target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
+target triple = "arm64--linux-gnu"
+
----------------
Remove the extra dash? 

================
Comment at: test/Transforms/SeparateConstOffsetFromGEP/AArch64/split-gep.ll:36
@@ +35,3 @@
+  %add = add i64 %shl, 12
+  %or = or i64 %add, 1 ; ((b << 2) + 12) and 1 have no common bits
+  %p = getelementptr float* %ptr, i64 %or
----------------
((a << 2) + 12)

http://reviews.llvm.org/D5863