[PATCH] D101701: [nofree] Refine concurrency requirements

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

In D101701#2744727 <https://reviews.llvm.org/D101701#2744727>, @jdoerfert wrote:

> Let me put down some thoughts:
>
> 1. What is the motivation here? I mean, which functions, except very few intrinsic, would be "strong" `nofree` but not `nosync`?

My main motivation here is to make the `nofree` attribute less surprising and easier to consume. I find having an attribute in the IR that is called "no free" but means "can free, unless some other attribute is present" pretty surprising.

It comes down to the ability to think about callees as abstracted black boxes: I care about the side effects of calling the black box; I don't care (and don't want to have to care) about how those side effects come to be.

> 2. Doesn't this just mean we would check for `nosync` in order to derive `nofree`? What would be different?

That is one option, though I believe it is slightly more conservative than it needs to be. See the inline discussion with @reames. (There may still be non-correctness arguments for going down the conservative path of course.)



================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:1282
+
+  if (I.hasReleaseOrdering()) {
+    if (auto *FI = dyn_cast<FenceInst>(&I))
----------------
reames wrote:
> This is wrong.  A thread can coordinate using only acquire ordering within 'f'.
> 
> Example:
> g = o; // the object being potentially freed
> if (g_acquire)
>   return; // then *caller* does release store
> use o;
> 
> The other thread does:
> while (!g) {
>   g_acquire = true;
>   while (!g_release) {}
>   free(g);
> }
> 
> Please exactly match the existing nosync logic in this file by using isOrderedAtomic helper in this file.  We can be more aggressive later if desired.
This is an interesting example, but I don't think it shows what you want it to show? Without the release store in the caller, the other thread cannot proceed to `free(g)`. In other words, the point at which `g`/`o` stops being dereferenceable is the release store in the caller, not the acquire load in the callee.

The acquire load is part of some coordination with the other thread, but it's redundant as far as the free concerned. At least it looks that way to me.

To attack this from a different angle, consider a slight variation of the example where the caller does a g_release store **before** calling f. In that case, putting `dereferenceable` on `o` is incorrect, because the other thread could free `o` before f is even entered.

>From yet another different angle, which part of the reasoning in the comment in the AANoFreeImpl is wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701