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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

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`.  That IR ^ could have resulted from:
>
>   int32* ptr0 = malloc(4);
>   int32* ptr1 = malloc(4);
>  
>   if (ptr0+1 != ptr1) return;
>  
>   int64 ptr0_i = (int64)(ptr0+1);
>   int64 ptr1_i = (int64)(ptr1);
>  
>   int32* ptr = (int*)ptr1_i;
>


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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59065