[PATCH] D19276: folding compares if pointers do not escape

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Minor nits inline.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1865
@@ -1864,1 +1864,3 @@
 
+static bool isFoldableCmpForUnescapedAlloc(Instruction *CmpOp) {
+  switch (CmpOp->getOpcode()) {
----------------
I'd make this more specific:  `isNeverEqualToUnescapedAlloc` (unless you want to extend this in ways that the  compare would fold to false).

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1871
@@ +1870,3 @@
+    LoadInst *LI = cast<LoadInst>(CmpOp);
+    // comparison to global pointer
+    if (isa<GlobalVariable>(LI->getPointerOperand()))
----------------
Nit: Capitalization in comment.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1873
@@ +1872,3 @@
+    if (isa<GlobalVariable>(LI->getPointerOperand()))
+      return true;
+    return false;
----------------
This could be `return isa<GlobalVariable>(LI->getPointerOperand());`

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1907
@@ +1906,3 @@
+        if (!isa<ConstantPointerNull>(ICI->getOperand(1))) {
+          Instruction *CmpOp0 = dyn_cast<Instruction>(ICI->getOperand(0));
+          Instruction *CmpOp1;
----------------
I'm not sure you need a `dyn_cast<>` here.  Have you tried just having `CmpOp0` be a `Value *`?

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1908
@@ +1907,3 @@
+          Instruction *CmpOp0 = dyn_cast<Instruction>(ICI->getOperand(0));
+          Instruction *CmpOp1;
+          if (CmpOp0 == PI)
----------------
`LHS` and `OtherOp` may be slightly better names than `CmpOp0` and `CmpOp1`.  Personally, I'd even elide `LHS` altogether, and use a ternary to compute the index of the "other" operand.

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1913
@@ +1912,3 @@
+            CmpOp1 = CmpOp0;
+          // we fold comparisons in some conditions provided the alloc has not
+          // escaped
----------------
Nit: capitalize comment


Repository:
  rL LLVM

http://reviews.llvm.org/D19276