[PATCH] D11443: Create constant GEP first to allow more LICM.

[hfinkel at anl.gov]

hfinkel added inline comments.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:747
@@ +746,3 @@
+  if (L && L->isLoopInvariant(ResultPtr) &&
+      NumOfCommonBaseGEPs[ResultPtr] == 1) {
+
----------------
Please comment on why you're only doing this for NumOfCommonBaseGEPs[ResultPtr] == 1.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:750
@@ +749,3 @@
+    // We perfer generate GEP with constant offset first if the other GEPs
+    // has loop variant operand and it deoesn't have foldable constant.
+    for (unsigned I = 1, E = Variadic->getNumOperands(); I != E; ++I, ++GTI) {
----------------
deoesn't -> doesn't

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:761
@@ +760,3 @@
+
+        GenerateConstFirst = true;
+        // If the other offset contain constant too, it could be folded later,
----------------
I'd rather you moved this to the bottom of the loop.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:762
@@ +761,3 @@
+        GenerateConstFirst = true;
+        // If the other offset contain constant too, it could be folded later,
+        // So we keep it in original order.
----------------
contain -> contains

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:763
@@ +762,3 @@
+        // If the other offset contain constant too, it could be folded later,
+        // So we keep it in original order.
+        if (BinaryOperator *BO = dyn_cast<BinaryOperator>(Idx))
----------------
So -> so

in original -> in the original

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:767
@@ +766,3 @@
+              dyn_cast<ConstantInt>(BO->getOperand(1)))
+            GenerateConstFirst = false;
+      }
----------------
And just made this a 'continue'

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:780
@@ +779,3 @@
+    Value *Offset = ConstantInt::get(IntPtrTy, AccumulativeByteOffset);
+    ResultPtr = Builder.CreateGEP(ResultPtr, Offset, "uglygep");
+  }
----------------
Why does this not have Builder.getInt8Ty() as its first argument like the one below? Could you introduce a small lambda function to avoid repeating the logic twice?

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:1063
@@ +1062,3 @@
+    Loop *L = LI->getLoopFor(B);
+    if (L && !LoopVisited[L]) {
+      LoopVisited[L] = true;
----------------
Seems like we:

 1. End up scanning loops we don't really care about at all (can't we do this lazily later?)

 2. Increment NumOfCommonBaseGEPs based on uses is separate loops; should the count be loop specific?



Repository:
  rL LLVM

http://reviews.llvm.org/D11443