[PATCH] Clarify wording in the LangRef around !invariant.load

[Philip Reames]

On 12/01/2014 11:17 AM, Andrew Trick wrote:
>> On Nov 24, 2014, at 2:35 PM, Sanjoy Das <sanjoy at playingwithpointers.com> wrote:
>>
>> Just to be clear: I have no problem with this change.  However, I
>> think the current semantics of invariant loads is counter-intuitive,
>> and should be clarified sometime in the future.
>>
>>> This is definitely valid:
>>>
>>> int *p = <known dereferenceable>
>>> m1 = load(p) !invariant
>>> store(q)
>>> m2 = load(p)
>>> if (something)
>>>   foo(m1)
>>> else
>>>   bar(m2)
>> I claim that this IR is only conditionally well-defined -- the
>> semantics say that "once the location is known dereferenceable along a
>> particular execution path that it's value is henceforth unchanging".
>> If p == q, and the value being stored by store(q) is not the same as
>> what p originally had, the program is ill-defined.  But it was also
>> ill-defined before in that case, so the transform itself is valid.
>>
>>> You would be forced to do this, which I think would prevent reordering store(q) and m1 = load(p):
>>>
>>> int *p = <known dereferenceable>
>>> store(q)
>>> if (something)
>>>   assume_invariant(p)
>>>   m1 = load(p) ;; ***
>>>   foo(m1)
>>> else
>>>   m2 = load(p)
>>>   bar(m2)
>>
>> My argument is that the above is the only intuitive interpretation of an
>> invariant load (if the *** load was marked invariant).  With the
>> current semantics of invariant loads, the following transforms (which
>> I believe are all correct and intuitive) make a well-defined program
>> undefined:
>>
>> %a = < known dereferenceable >
>> store 0, %a
>> store 1, %a
>> if ($false-at-runtime) {
>>   call undef(%a)  ;; well-defined, because it never executes
>> }
>>
>> suitable choice for undef, and inlining ==>
>>
>> %a = < known dereferenceable >
>> store 0, %a
>> store 1, %a
>> if ($false-at-runtime) {
>>   %b = load %a !invariant
>>   use(%b)
>> }
>>
>> to ==>
>>
>> %a = < known dereferenceable >
>> store 0, %a
>> %b = load %a !invariant
>> store 1, %a
>> if ($false-at-runtime) {
>>   use(%b) ;; this is still the original use undef(%b)
>>           ;; which initially was okay because it would never
>>           ;; execute
>> }
>>
>> to ==>
>>
>> %a = < known dereferenceable >
>> unreachable ;; modifying invariant location, "cannot happen"
>>             ;; since the original value at %a is either != 0
>>             ;; or != 1
> The "invariant" metadata property should be treated as alias analysis information. So storing to an location and invariant-loading from the same location would not be undefined behavior. The subsequent load could legally return either the stored value or the prior value (or poison if you like). As long as the value is never used, it doesn't affect semantics.
I agree with this.
>
>> As far as I can tell, there are three options for invariant_load:
>>
>>   1. we treat invariant_loads as side-effecting operations.  this
>>      prevents hoisting through control flow.
>>
>>   2. we accept this weird corner case of semantics not just following
>>      from what was actually executed, and teach the rest of the
>>      pipeline about it (so, for example, there are some values undef
>>      cannot be replaced by, and the first transform is declared
>>      illegal).
>>
>>   3. define a mutable location that has invariant loads in a way that
>>      the last transform is not sound (i.e. locations that have
>>      invariant loads can be modified), but I cannot think of a way to
>>      do that in a sound way.
>>
>>
>> The other option is to introduce a notion of an invariant attribute
>> that we can put on function arguments and return values (just like
>> nonnull).  This lets us mark incoming arguments as invariant
>> locations, and "make_invariant" is a just another function that has
>> its return value tagged with the "invariant" attribute.  This can then
>> be used exactly as you and Philip have suggested on this and other
>> threads.  Once that is in place, we can make the definition of an
>> invariant_load weaker, sound and just a convenient notation for
>>
>> int *p = <known dereferenceable>
>> store(q)
>> if (something)
>>   p1 = make_invariant(p)
>>   m1 = load(p1) ;; was m1 = load-invariant p
>>   foo(m1)
>> else
>>   m2 = load(p)
>>   bar(m2)
>>
>> — Sanjoy
> Until/unless we resolve the issue with llvm.invariant.end (do we need it), I don't think we can consider !invariant metadata as imbuing the pointer with invariance. (I raised this issue again in a separate response to Philip on an old llvm-dev thread).
One point of caution here: while !invariant.load and llvm.invariant.* 
are similar in intent, their actual implementations and specifications 
are currently completely independent.  I'm highlighting this since the 
current thread is *only* w.r.t. !invariant.load, *not* the others.

To date, nothing I've seen would lead me to believe that using an 
llvm.invariant.end and an !invariant.load on the same pointer value 
would be well defined.  They seem to conflict in their models.
>
> So, to reiterate, given:
>
> int *p = <known dereferenceable>
> store(q)
> if (something)
>    m1 = load(p) !invariant
>    foo(m1)
> else
>    m2 = load(p)
>    bar(m2)
>
> I claim this is clearly valid (and see no reasonable way for the optimizer to avoid it):
>
> int *p = <known dereferenceable>
> m1 = load(p) !invariant
> store(q)
> m2 = load(p)
> if (something)
>    foo(m1)
> else
>    bar(m2)
Agreed through here.
>
> But while I would really like to claim the following is also valid, I haven't made a complete argument for it yet. Given the current model, I don't think we can allow this in the optimizer:
>
> int *p = <known dereferenceable>
> m = load(p) !invariant
> store(q)
> if (something)
>    foo(m)
> else
>    bar(m)
I think this is completely legal for the current optimizer.  Here's my 
argument:
- !invariant.load describes the constant-ness of the *pointer value*.  
Once the pointer *value* is known dereferenceable, it is always invariant.
- The store implies dereferenceability.
- The store must (by assumption) be not changing the value of the 
location.  (Otherwise, the invariant.load marker lied originally.)
- Given the store can be completely omitted, combining the two loads 
becomes 'obviously' okay.

I'll agree that this is not the clearest semantics in the world, but it 
is consistent.
>
> That is, we cannot decompose !invariant metadata into a useful invariant intrinsic (either the current invariant.start or proposed assume_invariant or make_invariant) without defining stronger semantics and reasoning about safety.
This point I actually agree with, just not for the reason you've given.  :)
>
> -Andy
>