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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

>> 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)

For instance:

  // Let's say we know malloc(64) will always return a pointer that is 8 byte
  // aligned.
  
  int8* ptr0 = malloc(64);
  int8* ptr1 = malloc(64);
  
  int8* ptr0_end = ptr0 + 64;
  
  // I'm not sure if this comparison is well defined in C++, but it is well
  // defined in LLVM IR:
  if (ptr0_end != ptr1) return;
  
  intptr ptr0_end_i = (intptr)ptr0_end;
  
  intptr ptr0_end_masked = ptr0_end_i & -8;
  
  // I think the transform being added in this comment will fire below since it is
  // doing inttoptr(and(ptrtoint(ptr0_end), -8)).
  
  int8* aliases_ptr0_and_ptr1 = (int8*)ptr0_end_masked;

Right now `aliases_ptr0_and_ptr1` aliases both `ptr0` and `ptr1` (we can GEP backwards from it to access `ptr0` and forwards from it to access `ptr1`).  But if we replace it with `ptr0_end` then it can be used to access `ptr0` only.

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

That seems problematic for another reason:  IIUC you're saying `Alias(inttoptr(ptrtoint(X) & -8), A)` == `Alias(X, A)`.  But `X` is an illegal pointer so it does not alias anything (reads and writes on that pointer is illegal)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59065