[PATCH] D15146: [InstCombine] Look through PHIs, GEPs, IntToPtrs and PtrToInts to expose more constants when comparing GEPs

[David Majnemer via llvm-commits]

majnemer added a comment.

Please try to name your functions like `transformToIndexedCompare` instead of `TransformToIndexedCompare`.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:708
@@ +707,3 @@
+  }
+  if (Argument *A = dyn_cast<Argument>(V)) {
+    // Set the insertion point in the entry block.
----------------
Please use `auto *` here.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:835
@@ +834,3 @@
+    // a GEP or a GEP + ptrtoint.
+    if (dyn_cast<Instruction>(NewInsts[Val]))
+      setInsertionPoint(Builder, NewInsts[Val], false);
----------------
If you are not using the result of `dyn_cast`, use `isa` instead.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:861-863
@@ +860,5 @@
+                                                        const DataLayout &DL) {
+  Type *IndexType =
+      IntegerType::get((static_cast<Instruction *>(V))->getContext(),
+                       DL.getPointerTypeSizeInBits(V->getType()));
+
----------------
The caller of `GetAsConstantIndexedAddress` passes in a `GEPOperator` but you are casting it to an `Instruction`.  How have you ensured that the `GEPOperator` is an `GetElementPtrInst` instead of a `ConstantExpr` ?

Also, `Value` has a `getContext` member.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:881
@@ +880,3 @@
+    }
+    if (IntToPtrInst *CI = dyn_cast<IntToPtrInst>(V)) {
+      if (DL.getPointerTypeSizeInBits(CI->getType()) !=
----------------
Please use `auto *CI` here.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:881-894
@@ +880,16 @@
+    }
+    if (IntToPtrInst *CI = dyn_cast<IntToPtrInst>(V)) {
+      if (DL.getPointerTypeSizeInBits(CI->getType()) !=
+          CI->getOperand(0)->getType()->getScalarSizeInBits())
+        break;
+      V = CI->getOperand(0);
+      continue;
+    }
+    if (PtrToIntInst *CI = dyn_cast<PtrToIntInst>(V)) {
+      if (CI->getType()->getScalarSizeInBits() !=
+          DL.getPointerTypeSizeInBits(CI->getOperand(0)->getType()))
+        break;
+      V = CI->getOperand(0);
+      continue;
+    }
+    break;
----------------
majnemer wrote:
> Please use `auto *CI` here.
Can you use `isNoopCast` here?  Should simplify things.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:888
@@ +887,3 @@
+    }
+    if (PtrToIntInst *CI = dyn_cast<PtrToIntInst>(V)) {
+      if (CI->getType()->getScalarSizeInBits() !=
----------------
Ditto.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:903
@@ +902,3 @@
+// common base between GEPLHS and RHS.
+Instruction *TransformToIndexedCompare(GEPOperator *GEPLHS, Value *RHS,
+                                       ICmpInst::Predicate Cond,
----------------
Shouldn't this function be `static` ?


http://reviews.llvm.org/D15146