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

Sun Sep 3 23:40:17 PDT 2017

On Sun, Sep 3, 2017 at 10:03 PM, Hal Finkel <hfinkel at anl.gov> wrote:

>
> On 09/03/2017 11:35 PM, Daniel Berlin wrote:
>
>
>
> On Sun, Sep 3, 2017 at 7:52 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>
>>
>>
>> On Sun, Sep 3, 2017 at 7:17 PM, Daniel Berlin <dberlin at dberlin.org>
>> wrote:
>>
>>>
>>>> Sanjoy's description of the current behavior is accurate.
>>>>
>>>
>>>
>>>
>>>> I see two ways of looking at this:
>>>>
>>>>  1. This is a bug.
>>>>  2. To allow this kind of inter-object addressing you need to use
>>>> inttoptr/ptrtoint.
>>>>
>>>>
>>> I'm fine with #2, i just feel like our rules are a little hodgepodge:)
>>>
>>>
>>>> I don't believe that it is possible for the current behavior to
>>>> manifest as a problem for C/C++ (and this is likely the same for most
>>>> higher-level languages).
>>>>
>>>
>>> I don't believe it is either - we've already stated we assume aliasing
>>> does not imply pointer equality and non-aliasing does not imply pointer
>>> inequality.
>>> If we did, this behaviour could.
>>>
>>
>> Which means the weirdest discontinuity that occurs to me off the top of
>> my head is that we are treating malloc's like lifetime beginning operations
>> in a bunch of cases (GVN does this) , but if you call free on the pointer
>> from the gep in sanjoy's example, there is no way to know it could have
>> ended the lifetime of both p0 and p1 without a separate analysis.
>>
>> It means things like this in memorydependenceanalysis:
>>
>>   if (const CallInst *CI = isFreeCall(Inst, &TLI)) {
>>     // calls to free() deallocate the entire structure
>>     Loc = MemoryLocation(CI->getArgOperand(0));
>>     return MRI_Mod;
>>   }
>> will say this free does not touch p1, even though it could have destroyed
>> it and ended the lifetime.
>>
>> Do we have anything that takes advantage and does something silly? I
>> don't think we have anything that advanced ATM.
>>
>>
> Actually, this is, IMHO,  worse than i initially thought.
>
> So imagine we free %gep from sanjoy's example.
>
> Right now, the above code (and elsewhere) will say that free does not MOD
> %p1.  Even if the pointer is value equal to %p1
>
> We already say malloc operates by returning new memory regardless of what
> pointer value it returns.  Here, free is taking a pointer and we  are
> saying it only modifies the memory that pointer can alias.
>
> I can think of a few possibilities
>
> 1. Free operates by pointer value and not "abstract memory location", in
> which case, the above code is wrong, as free will modify things that are
> value equal, even if they do not alias.
>
>
> I don't believe we can have this model. We model free, I believe fairly,
> as writing to the memory being freed.
>

I'm going to leave whether that's correct for another day, but if you want
to be pedantic, all C11 says is:
"The free function causes the space pointed to by ptr to be deallocated,
that is, made
available for further allocation. "

(I have no urge here to go down the rabbit hole of whether you can free
pointers you couldn't load and store due to TBAA or whatever)

Thus, we need to following aliasing rules regarding what it can affect.
>

Okay.

> Otherwise, we'd need to model free as potentially modifying anything
> (because we'd have no infrastructure to deal with figuring out what it
> might affect).
>
> Correct.

> In this model, except through value numbering or range analysis, you can't
> really ever tell what free has done, it may mod/ref anything (and we have
> some bugs to fix).
>
> Also now malloc returns abstract memory objects, but free doesn't really
> destroy them
>
> 2. An attempt by free to destroy p1 is considered UB, and sanjoy's code,
> with a free at the end cannot validly destroy p1.
>
>
> This is what we have, as far as I can tell. It is true that you can't have
> an additional call to free(%p1) at the end, but that's irrelevant, as UB
> would have already occurred.
>

So, i instrumented llvm to add runtime asserts about the noaliasing
pointers before free (IE it adds an assert that, for each pointer that is
noaliased with the pointer passed to free, free is not destroying that
pointer), and it fails really quickly.

Hence, i'm going to go with "i still have trouble believing we are
generating currently correct code that accounts for this model".
Though i admit nothing currently takes advantage of free that i can find.
But at least i feel like i understand what we are trying to model :)

>

>

>
>   I would have trouble believing we are generating currently correct code
> in this model from any of our frontends, but willing to be convinced (IE i
> would believe if i added asserts to free calls that they are not value
> equal to anything llvm considers the argument to noalias, it would fail)
>
> 3. Free is not mapped into our memory model, but malloc is.
>
>
> I'm not sure what this means.
>

It means unlike malloc, which we've taken from c's memory model and said
things about what it does in llvm's, we have decided not to do anything
with free
(IE attempts to rely on saying anything about llvm memory objects based on
free is nonsense)

>
>
> Thoughts? Other possibilities?
>
>
> Referring to this:
>
> define void @f(i64 %arg, i64 %arg1) {
> bb:
>   %p0 = tail call noalias i8* @malloc(i64 %arg) #3
>   %p1 = tail call noalias i8* @malloc(i64 %arg) #3
>   %offset = tail call i64 @subtract(i8* %p1, i8* %p0)
>   %gep = getelementptr i8, i8* %p0, i64 %offset  ;; not inbounds
>   ret void
> }
>
> declare noalias i8* @malloc(i64)
> declare i64 @subtract(i8*, i8*)
>
> In our model, as I understand it, free of %gep: As %gep is based on %p0,
> and free must be called with a pointer to the beginning of the allocation,
> %offset must be 0. If offset is not 0, the behavior is undefined.
>

Then, as i said, i think we've got a lot of cases we aren't generating
correct code (though again, nothing is trying to take a lot of advantage of
free).
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170903/957a5553/attachment-0001.html>