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

[Andrew Trick]

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

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

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)

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)

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.

-Andy