[PATCH] D101701: [nofree] Refine concurrency requirements

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D101701#2747492 <https://reviews.llvm.org/D101701#2747492>, @nlopes wrote:

> In D101701#2746949 <https://reviews.llvm.org/D101701#2746949>, @jdoerfert wrote:
>
>> In D101701#2746841 <https://reviews.llvm.org/D101701#2746841>, @nlopes wrote:
>>
>>> @jdoerfert a function that syncs with another thread may not free anything itself and the other thread may not free anything either.
>>> If you require an optimization to have nofree + nosync in a call to preserve dereferenceability then you can't optimize this case.
>>
>> I disagree, and I think this is the conceptual differences we have here.
>>
>> If one does only look at a function in isolation then I agree, `nofree` alone is not sufficient to make statements about all call sites and arguments.
>> However, if you look at a call site of a `nofree` function the situation is different because you have context information about the arguments.
>>
>> Let's go back to my last example:
>>
>>   a = malloc
>>   b = malloc
>>   unknown(a);                  // no attributes 0-> escapes
>>   nofree_nocapture_only(a, b); // `nofree`, `nocapture` on args
>>
>> I know that at the end of this sequence `b` has not been freed, even though there is no `nosync` in sight.
>> I can also derive `nofree` for `nofree_nocapture_only` without worrying about atomics, for example.
>> (Here is the sequence in action with heap2stack kicking in for b: https://godbolt.org/z/hjYrhaEvW)
>>
>> So, with `nofree` and `nosync` being completely separated, as it was in the very beginning, you can:
>>
>> 1. Optimize even if only one is present, given additional call site information.
>> 2. Derive `nofree` even if `nosync` is not present.
>
> Your examples are around nofree arguments, not nofree functions, which is what this patch is about.

`nofree` functions imply `nofree` arguments so it very much makes a difference if we make it harder to infer
and annotate the former. As we already moved to a more semantic-based definition of escaped we can actually
look at an example where this makes a difference. Take pointers passed via memory to see how you cannot just
 move the `nofree` to account for the change made by this patch:

  struct { int *a } s;
  s.a = malloc()               // this is a store.
  nofree_nocapture_only(&s)    // we can potentially show `s.a` is not captured in the function but we cannot
                               // annotate that fact as a `nofree` argument without argument promotion, which
                               // we cannot always do.
  // s.a is still known to be dereferenceable even without `nosync`. The function `nofree` is needed, `nofree`
  // argument attributes are not involved.



> nofree in functions is useful for pointers that are *not* passed as argument, like this:
>
>   f(i8* %p) {
>     load %p  ; implies it's dereferenceable
>     call @g() nofree
>     ; is %p still dereferenceable or not?
>   }
>
> This patch states that yes, %p is still dereferenceable after the call to @g as it's tagged with nofree. That's it.

Let's not ignore the arguments that are somehow passed though. Anyway, this patch does not actually add anything you
need for you example. If `g` is `nofree` & `nosync` you get exactly the same effect already. I hope we agree on that.

If we allow user feedback you can also make your `@f` `nosync` and get the effect you want even if `@g` is not `nosync`,
This is especially useful if @g is an intrinsic users cannot actually annotate.

In addition to the examples I brought up that are impacted, let me reiterate what I said in my first post:
Other attributes work in the same composable way:

  void foo(int * __restrict__ flag, char* p) {
    argmemonly(flag);                 // Says argmemonly and p is not passed nor does p alias flag, still p is potentially written.
                                      // This is like a call to a `nofree` function which might still free pointers via synchronization.
                                      // `nosync` is a orthogonal component to these attributes as we can otherwise not derive them.
                                      // However, as with `nofree`, we can use the attributes even without `nosync` in some situations.
  }




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701