Stream: t-compiler/wg-llvm

Topic: deferencable attribute #66600


pnkfelix (Nov 21 2019 at 09:51, on Zulip):

Hey there @RalfJ , I realized maybe i'd be better off having dialogue here rather than via github comments. :)

pnkfelix (Nov 21 2019 at 09:52, on Zulip):

(that is, I think you and I are skimming the same LLVM PR's and/or email threads at the moment, such as LLVM D61652 "Introduce dereferenceable_globally", so maybe its easiest to discuss live)

pnkfelix (Nov 21 2019 at 09:54, on Zulip):

(Also, it does look like they landed nofree in D49165?)

rkruppe (Nov 21 2019 at 09:59, on Zulip):

One concern Ralf raised in the past is that nofree and nosync are function attributes, so if a function frees any memory (e.g. because it creates a temporary Vec internally) or synchronizes with anything (e.g. because it may panic, which involves at least some atomics to access the panic hook unless it's a no_std binary), no dereferenceable information for any pointer can be propagated across calls to it. My gut feeling is to agree that could potentially cause quite some regressions.

pnkfelix (Nov 21 2019 at 09:59, on Zulip):

I just noticed that point in the comment history. :)

pnkfelix (Nov 21 2019 at 10:02, on Zulip):

Still, I don't really understand the argument being made here. That is: I can understand making the argument "the model you are adopting is not as expressive as it could be."

pnkfelix (Nov 21 2019 at 10:02, on Zulip):

but that statement should not imply "it will regress performance from what we have today", as long as the future model is at least as expressive as today's model

pnkfelix (Nov 21 2019 at 10:03, on Zulip):

The only way I can see it regressing performance (and maybe this is what you all are in fact saying) is that if they fix bugs in the existing optimizations via relying on these additional attributes

pnkfelix (Nov 21 2019 at 10:03, on Zulip):

i.e. that we are today relying on optimizations that are themselves unsound (or that we are generating code that is generally-incorrectly attributed, and that code is then optimized)

pnkfelix (Nov 21 2019 at 10:05, on Zulip):

So is that the explanation for where the expected performance regressions would come from? Or am I misunderstanding?

pnkfelix (Nov 21 2019 at 10:08, on Zulip):

okay I now think I see how it all fits together

rkruppe (Nov 21 2019 at 10:08, on Zulip):

It depends a bit on how the old dereferenceable attribute on function parameters is interpreted. If (as we generally did in the past) it means "dereferenceable for the duration of this function", that is something that's generally sound for e.g. Rust references passed as parameters (at least if there's no interior mutability) but can't be expressed in the future model. The new dereferenceable won't assert as much as it previously did due to nosync and nofree not being as precise (see above), and dereferenceable_globally is too strong (the referent can be deallocated after the function returns).

pnkfelix (Nov 21 2019 at 10:08, on Zulip):

okay yes, thanks for spelling that out

rkruppe (Nov 21 2019 at 10:08, on Zulip):

If on the other hand "the old dereferenceable" was supposed to mean what's now dereferenceable_globally then our usage of dereferenceable was always incorrect.

pnkfelix (Nov 21 2019 at 10:13, on Zulip):

Am I right in thinking that what we may want is something like the derefenceable_in_scope semantics (perhaps in its own dedicated attribute), described here ?

pnkfelix (Nov 21 2019 at 10:14, on Zulip):

or maybe I've misunderstood how the patch developed over time

pnkfelix (Nov 21 2019 at 10:15, on Zulip):

in any case, I think I am now semi-convinced that @RalfJ was not wrong to raise a concern on that thread

rkruppe (Nov 21 2019 at 10:16, on Zulip):

It seems like that is what we would want, but LLVM folks had concerns about whether those semantics can be implemented soundly (while still being useful for optimizations) so the patch changed to dereferenceable_globally.

rkruppe (Nov 21 2019 at 10:21, on Zulip):

Ah, but after digging out the old diff, I see it tried to be clever about defining the attribute in such a way that it can apply to a region within a function. That's generally always very hard to do correctly (& in a way that plays nicely with other transformations). Defining that's tied to function scope and dropped when inlining/outlining/etc. is much easier and would get us at least some fraction of the potential optimizations.

pnkfelix (Nov 21 2019 at 10:21, on Zulip):

So our running assumption is that the "new" interpretation of the deferenceable attribute is going to be "this is dereferenceable on function entry alone", and it will be up to LLVM analyses to determine how that affects the rest of the body?

rkruppe (Nov 21 2019 at 10:22, on Zulip):

Yes

rkruppe (Nov 21 2019 at 10:23, on Zulip):

And FWIW it's defined in a way that we can also translate e.g. let x: &T = foo(); to a call with dereferenceable attribute on the return value and that means the returned pointer is dereferenceable when it's returned (and analyses can try to figure out how long this holds from that point onward).

pnkfelix (Nov 21 2019 at 10:27, on Zulip):

yeah, I now see the overall problem. I had thought on my initial reading that the no-free attribute was specified on a parameter to a function, not as a property of the function as a whole

pnkfelix (Nov 21 2019 at 10:28, on Zulip):

(and that no-free would mean something like "the data pointed at by the parameter is not freed during the function's invocation.")

pnkfelix (Nov 21 2019 at 20:20, on Zulip):

yeah, I now see the overall problem. I had thought on my initial reading that the no-free attribute was specified on a parameter to a function, not as a property of the function as a whole

hmm, LLVM dev jdoerfert's response to @RalfJ leads me to think that my original interpretation was correct...? That is, no-free is a property on a parameter, not necessarily the function definition as a whole?

rkruppe (Nov 21 2019 at 21:11, on Zulip):

That is also my reading of that comment, but the nofree patch I'm aware of (https://reviews.llvm.org/D49165) explicitly defines it as a function attribute. Perhaps there was another patch?

rkruppe (Nov 21 2019 at 21:12, on Zulip):

Yes, seems so: https://github.com/llvm/llvm-project/blob/44fe1f024d542bb7d286f9dd03ef35ad474399bd/llvm/docs/LangRef.rst#L1136

rkruppe (Nov 21 2019 at 21:14, on Zulip):

https://reviews.llvm.org/D67886

pnkfelix (Nov 22 2019 at 13:29, on Zulip):

Yes, seems so: https://github.com/llvm/llvm-project/blob/44fe1f024d542bb7d286f9dd03ef35ad474399bd/llvm/docs/LangRef.rst#L1136

I'm not an expert in reading these docs. What does this imply if the argument points to aliased memory? I.e. if I take two pointer parameters x and y, and I mark x nofree, but then I do free y?

pnkfelix (Nov 22 2019 at 13:30, on Zulip):

My hope/intuition is that writing such code is a violation of the contract of nofree

rkruppe (Nov 22 2019 at 13:49, on Zulip):

Yes, that seems pretty clear-cut to me. The attribute asserts (implied: under threat of UB) that the memory pointed to by x is not freed by the callee, so any execution where that memory is freed is UB, no matter what pointer was used to free it.

RalfJ (Nov 22 2019 at 14:03, on Zulip):

yeah I am confused now, too... nofree has all the same scope issues as dereferencable_in_scope, one would think

RalfJ (Nov 22 2019 at 14:03, on Zulip):

I haven't had time to take another look at this

rkruppe (Nov 22 2019 at 14:18, on Zulip):

I don't think it's comparable. For reference, the proposed definition of dereferenceable_in_scope was:

This indicates that the annotated pointer value has the dereferenceable(<n>) property at any program point in its scope (as defined by the SSA dominance property). Thus, unlike pointer values annotated with dereferenceable(<n>), dereferenceable_in_scope(<n>) pointer values can never lose the dereferenceable(<n>) property as long as the annotated value is accessible as an SSA value.

This is problematic because it's very easy and common to expand the dominance region of values (even if you're not even looking at that value at the moment), and it's not natural or expected that this would have any repercussions. An attribute tied specifically to function boundaries, on the other hand, only requires care when changing function boundaries, which is a concern for far fewer transformations. It's also mechanically easier to ensure you drop the attribute when it becomes invalid (e.g. when inlining, you're replacing uses of the parameter anyway, so you'd have to go out of your way to introduce a bug by restoring the attribute afterwards).

RalfJ (Nov 22 2019 at 16:04, on Zulip):

ah, so nofree only works on function parameters... but then one could do that for dereferncable_in_scope as well?

rkruppe (Nov 22 2019 at 16:17, on Zulip):

Yes, I even mused about that possibility earlier in this thread. But if nofree + dereferenceable-at-entry is sufficient (is it? I'm not sure) why introduce yet another attribute?

RalfJ (Nov 22 2019 at 17:46, on Zulip):

not sure if it is... concurrency is another concern

RalfJ (Nov 22 2019 at 17:47, on Zulip):

inserting spurious writes that you can prove would happen anyway later would not be allowed because they might introduce a race

RalfJ (Nov 22 2019 at 17:47, on Zulip):

moving reads around might turn them from non-racy to racy thus returning wrong results

rkruppe (Nov 22 2019 at 18:00, on Zulip):

And there's not really a good way to use nosync to fix that, is implied? That seems plausible. nosync is kinda "all or nothing" (not per-pointer) and we can't apply it to most functions.

RalfJ (Nov 22 2019 at 18:40, on Zulip):

well I thought nofree was global

RalfJ (Nov 22 2019 at 18:41, on Zulip):

IOW, nosync could be per-ptr, too... ah but that won't help, it could be other ptrs doing the sync

rkruppe (Nov 23 2019 at 13:08, on Zulip):

For &T references without interior mutability, shouldn't the noalias readonly we add ensure that there are no writes (from any thread, least of all another) for the duration of the function? That won't help for &mut T, but for those, both hoisting loads and spurious writes generally requires aliasing information (again: no intervening accesses from any thread, including the current one) in addition to dereferenceability.

RalfJ (Nov 25 2019 at 13:12, on Zulip):

hm, that sounds right, yes

Last update: Dec 12 2019 at 01:40UTC