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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for taking a look!

In D59065#1448677 <https://reviews.llvm.org/D59065#1448677>, @sanjoy wrote:

> In D59065#1448178 <https://reviews.llvm.org/D59065#1448178>, @hfinkel wrote:
>
> > > I am also not entirely sure how control dependencies could add new underlying objects with this patch
> >
> > Please read this https://bugs.llvm.org/show_bug.cgi?id=34548
> >
> > I *think* that this is okay if the ptrtoint and the and have only one user, and they're in the same basic block, and there's nothing that can throw, etc. in between?
>
>
> I'm not sure about this.  You could have:
>
>   int32* ptr0 = malloc(4);
>   int32* ptr1 = malloc(4);
>  
>   if (ptr0+1 != ptr1) return;
>  
>   int32* ptr = (int*)(int64)(ptr0+1);
>
>
> in which `ptr` would alias `ptr1`.  But if you transform `ptr` to `ptr0+1` then it would not alias `ptr1`.


I am probably missing something, but I am not sure how such a case would be possible with this patch? It specifically looks for a inttoptr(and(ptrtoint, C)) sequence, where C is such that the (logical) destination of the pointer remains unchanged. Unfortunately I do not think the LangRef is clear about 'irrelevant' bits in pointers (due to alignment or address spaces)

In D59065#1448739 <https://reviews.llvm.org/D59065#1448739>, @aqjune wrote:

> I just saw this patch. I agree with Sanjoy's comment.
>  One possible workaround to allow this optimization is to check dereferenceability of the original pointer:
>
>   p1 = malloc()
>   v16 = ptrtoint p1 to i64
>   p2 = inttoptr v16 to i8*
>   store i8* p1, 10
>   store i8* p2, 20
>   =>
>   p1 = malloc()
>   v16 = ptrtoint p1 to i64
>   p2 = inttoptr v16 to i8*
>   store i8* p1, 10
>   store i8* p1, 20 // p2 replaced with p1
>
>
> If (1) the original pointer p1 has been dereferenced before, and (2) there have been no operation that may have freed p1, we can assume that p2 must alias p1.
>  Informal reasoning is as follows. It should be guaranteed that replacing p2 with p1 does not introduce undefined behavior.  If p1 already have been dereferenced before, replacing it does not introduce new undefined behavior.
>
> Does this workaround work for this patch?


In the example, the original pointer (`%addr = load %struct.zot*, %struct.zot** %loc, align 8`) is not dereference directly and the use case I am looking at is tagged pointers, where the inttoptr(and(ptrtoint(), C) roundtrip is required to get a valid pointer.  So the original pointer might not be dereferenceable directly, but logically (ignoring the bits irrelevant for the pointer value) it should still point to the same object.  Does that make sense to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59065