[PATCH] D37419: Teach scalar evolution to handle inttoptr/ptrtoint

[Sanjoy Das via llvm-commits]

Hi,

+CC llvm-commits

On Sun, Sep 3, 2017 at 4:01 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> On Sun, Sep 3, 2017 at 12:09 PM, Sanjoy Das via Phabricator via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>>
>> sanjoy requested changes to this revision.
>> sanjoy added a comment.
>> This revision now requires changes to proceed.
>>
>> Hi David,
>>
>> Generally it is not safe to look through `inttoptr` and `ptrtoint` in
>> SCEV.
>> https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ScalarEvolution.cpp#L6053
>> has a little bit of the rationale, but basically the problem is that in:
>>
>>   %i0 = ptrtoint i8* %p0 to i64
>>   %i1 = ptrtoint i8* %p1 to i64
>>   %i2 = add i32 %i0, %i1
>>   %ptr = inttoptr i64 %i2 to i8*
>>
>> `%ptr` can alias both `%p0` and `%p1`.
>
>
> Okay, AFAICT, given p0 noalias p1, it can really alias the disjunction of p0
> and p1, right?
>
> IE either p0 is null or p1 is null, and then it aliases the other one. It
> doesn't really alias both at the same time, it's just you can't tell which
> of them it aliases statically.

Yes.  I should have said "`%ptr` is may-alias with both `%p0` and
`%p1`" which would have made this clearer.

>> However, if you round-trip `%ptr` through SCEV and SCEVExpander, then
>> nothing stops SCEVExpander from expanding the SCEV for `%ptr` to
>>
>>   %i0 = ptrtoint i8* %p0 to i64
>>   %i1 = ptrtoint i8* %p1 to i64
>>   %p0_ = inttoptr i64 %i0 to i8*
>>   %ptr = getelementptr i8, i8* %p0_, i64 %p0
>>
>
> I assume you meant to index it by  something else, since this index is not
> an i64?

Yes, as David already pointed out, this should have been `%ptr =
getelementptr i8, i8* %p0, i64 %i1` (or `%ptr = getelementptr i8, i8*
%p0_, i64 %i1`, both expansions have the same problem).

> I also don't understand "  // It's tempting to handle inttoptr and ptrtoint
> as no-ops, however this can
>   // lead to pointer expressions which cannot safely be expanded to GEPs,
>   // because ScalarEvolution doesn't respect the GEP aliasing rules when
>   // simplifying integer expressions."
>
> 1. What are "the gep aliasing rules", exactly?
> https://llvm.org/docs/LangRef.html#pointeraliasing does not mention special
> rules for gep :)

I think the rule it is talking about is that reads and writes on a GEP
are allowed to only read from and write to the allocation that is
passed in as the first parameter to the GEP -- you can't GEP from one
allocation to another different allocation and issue reads/writes.  So
my statement above was imprecise, `%p0` no-alias `%p1` is not
sufficient to infer no-alias in the GEP case, you'd have to know that
`%p0` and `%p1` are different allocations (based off different malloc
or alloca, for instance).

I think the pointer aliasing rules have a typo (which I'll fix), but
in principle this follows from:

"Any memory access must be done through a pointer value associated
with an address range of the memory access, otherwise the behavior is
undefined. Pointer values are associated with address ranges according
to the following rules"

"A pointer value is associated with the addresses associated with any
value it is based on."

"A pointer value formed from a getelementptr operation is based on the
second [should be *first*] value operand of the getelementptr."

> 2.  Even ignoring "the gep aliasing rules" i don't see how this is ever a
> correct expansion of p0.

Yes, that's a copy/paste typo on my part.  I should have used %i1 as the index.

-- Sanjoy