[PATCH] D37832: Eliminate PHI (int typed) which is only used by inttoptr

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:101-103
+  if (DL.getPointerSizeInBits(IntToPtr->getAddressSpace()) !=
+      DL.getTypeSizeInBits(IntToPtr->getOperand(0)->getType()))
+    return nullptr;
----------------
wmi wrote:
> Is this check always false? IntToPtr->getOperand(0)'s type is a pointer type, its type size should be equal to DL.getPointerSizeInBits?
IntToPtr's operand 0's type is integer type not pointer type.


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:157-158
+  auto *BB = PN.getParent();
+  assert(AvailablePtrVals.size() == PN.getNumIncomingValues() &&
+         "Not enough available ptr typed incoming values");
+  PHINode *MatchingPtrPHI = nullptr;
----------------
wmi wrote:
> There are four case in which Arg will be added to AvailablePtrVals: 1. Arg comes from ptrtoint 2. Arg is used by inttoptr dominating the incoming bb. 3. Arg is a phi. 4. Arg is a load. 
> Seems it is still possible for Arg not to fall into the cases above, like Arg is just an int parameter. Then the assertion may break?
The argument case requires ArgIntPtr to be nonull , so it will by pass the rest of the loop (line 127 above).


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:160
+  PHINode *MatchingPtrPHI = nullptr;
+  for (auto II = BB->begin(), EI = BB->end(); II != EI; II++) {
+    PHINode *PtrPHI = dyn_cast<PHINode>(II);
----------------
wmi wrote:
> Use EI = BB->getFirstNonPHI()  to only iterate phi instructions?
Ok.


https://reviews.llvm.org/D37832