[PATCH] D61652: [Attr] Introduce dereferenceable_globally

[Robin Kruppe via Phabricator via llvm-commits]

rkruppe added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1170
+    ``dereferenceable(<n>)`` property *at any program point*, starting from the
+    definition of the value to the termination of the program. Thus, unlike
+    pointer values annotated with ``dereferenceable(<n>)``,
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > RalfJung wrote:
> > > In Rust, we are using the existing  `dereferenceable` attribute on function parameters, and we are generally happy with its semantics, which we interpreted to be "this pointer is dereferenceable for the entire duration of this function call". We automatically set this attribute for all references passed to a function, which in Rust (unlike in C++) are guaranteed to not be deallocated while the function is ongoing.
> > > 
> > > However, the new `dereferenceable_globally` that is being proposed here is useless for us: even memory pointed to by references *does* eventually get deallocated. We hence cannot set that attribute. So, the patch as proposed constitutes a big regression in terms of the kind of information that the Rust frontend can provide to LLVM.
> > My first thought: `dereferenceable` + `nofree` on the argument should give you the semantics you think ` dereferenceable` has now.
> > 
> > I will ping you once I rebase the updated users of ` dereferenceable` which will then look at the new ones (`nofree`, ` dereferenceable_globally`, ..) as well.
> Slight correction: 
> `dereferenceable` + `nosync` (on the function) + `nofree` (on the arg or function) should do it, same as 
> `dereferenceable` + `noalias` (on the argument) + `nofree` (on the argument or function)
> 
> We might want to have a `nosync` on an argument as well to allow fine granularity here.
Rust can't really emit `nosync`, at least not without doing just as much IPO and analysis as LLVM would have to infer it by itself (and failing to infer it at least as often). Nothing about the language rules out synchronization happening anywhere. Furthermore, the majority of Rust code can't possibly be `nosync` even with perfect attribute inference, because a ton of code can potentially panic and (in most configurations) panicking involves some synchronization.

I also doubt `nosync` can usefully be made more granular (e.g. restricted to individual allocations): happens-before is a superset of inter-thread program order, so any synchronization at all will make all previous non-atomic stores visible to other threads, regardless of what parts of memory they affected.

Luckily, the second option (`dereferenceable` + `noalias` + `nofree` on argument) is feasible for Rust. At least for immutable references. There are too many miscompiles with `noalias`+mutation, so we don't currently add `noalias` or anything like that to mutable references. That needs to be fixed eventually, but as long as it lasts, we can't emit `dereferenceable`+`noalias`+`nofree` for mutable references. But maybe it won't be worse than before the change to `dereferenceable`, since exploiting dereferenceability is difficult without aliasing information in any case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61652/new/

https://reviews.llvm.org/D61652