[PATCH] D101701: [nofree] Refine concurrency requirements

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D101701#2746437 <https://reviews.llvm.org/D101701#2746437>, @nhaehnle wrote:

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

But all our attributes work that way so changing `nofree` would make it an outlier.
Some examples:

  willreturn + nounwind -> comes back and executes the next instruction after the call
  argmemonly/inaccessiblemem_only + nosync -> there is no synchronization that revealed new values for globals

Depending on where we did fall wrt. synchronizing intrinsics you really cannot even use `readnone/readonly`
without `nosync` (maybe restricted to convergent functions).

The reason we want this is that attributes should compose. Almost always they also help on their own.
`nofree` without `nosync` can in fact be used for optimization. However, if we force it to go hand
in hand we loose that. Let me show you by example:

  a = malloc
  b = malloc
  unknown(a);
  nofree_nocapture_only(a, b);

Here, I can do heap2stack for b but not for a because `nofree_nocapture_only` could synchronize with someone to free `a` which escaped before.

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

I do not understand how this would be affected. The attributes limit the effects of the black box, regardless if there is one or two.
Could you elaborate why the black box view would be impaired if we do not apply this change?

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

The only more conservative part I can see is arguably out of range for the static analyses we have right now.
That is, we could add "strong nofree" if we show that all synchronizing parties do not free. Other than that
I fail to see how this is not just `nofree` + `nosync` packaged as `nofree`, and that does not make sense to
me at all. Composability is useful, as shown above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701