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

[Wei Mi via Phabricator via llvm-commits]

wmi 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;
----------------
Is this check always false? IntToPtr->getOperand(0)'s type is a pointer type, its type size should be equal to DL.getPointerSizeInBits?


================
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;
----------------
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?


================
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);
----------------
Use EI = BB->getFirstNonPHI()  to only iterate phi instructions?


================
Comment at: lib/Transforms/InstCombine/InstCombinePHI.cpp:173
+    if (MatchingPtrPHI)
+      break;
+  }
----------------
Don't have to break then return below. We can just return here.


https://reviews.llvm.org/D37832