[PATCH] D59065: [BasicAA] Simplify inttoptr(and(ptrtoint(X), C)) to X, if C preserves all significant bits.

Thu Mar 14 11:51:44 PDT 2019

fhahn marked an inline comment as done.
fhahn added a comment.

In D59065#1429435 <https://reviews.llvm.org/D59065#1429435>, @hfinkel wrote:

> I'm really afraid that this isn't sound. We've had a number of issues in this space, and we've always resisted attempts to make BasicAA look through inttoptr/ptrtotint. Once you convert to an integer, control dependencies can effectively add additional underlying objects. In cases where this is sound, why not fold away the whole inttoptr(and(ptrtoint)) in the first place?

Thanks. I agree it is a bit shaky as I think the  LangRef is not too clear about the issue. I think it would make sense to specify that the unused bits have to be 0 for memory accesses (at least for MacOS/Linux on Arm64/X86), but maybe not for getelementptr & co. Given how BasicAA is used to look through getlementptrs, I am not entirely sure that can be guaranteed at the moment.

I am also not entirely sure how control dependencies could add new underlying objects with this patch. We are looking backwards for a specific pattern and  the underlying object has to dominate the and & ptrtoints.

In the test case, it is not possible to fold to inttoptr(and(ptrtoint)) unfortunately, because the original pointer has some unused bits set, and they need to be cleared before memory accesses, otherwise an invalid address is accessed.

================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:3760
+        uint64_t MeaningfulPtrBits =
+            (AllOnes >> (64 - DL.getMaxPointerSizeInBits())) &
+            (AllOnes << Log2_64(DL.getMinPointerABIAlignment()));
----------------
arsenm wrote:
> I don't see why you would use the max size, when you can check the 2 address space sizes
Thanks, I'll update the code, if the underlying code is sound.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59065/new/

https://reviews.llvm.org/D59065