[PATCH] D101701: [nofree] Refine concurrency requirements

Johannes Doerfert via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed May 12 11:05:48 PDT 2021


jdoerfert requested changes to this revision.
jdoerfert added a comment.
This revision now requires changes to proceed.

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

>>> 3. Today's `nofree` enables annotations and optimizations that the proposed change would no longer allow. I can't say for certain whether this is false, but at least the examples given so far seem to fail (the first one because you can just use the `nofree` **argument attribute** instead, and the second is discussed above in this comment).
>>
>> I think once I manage to explain the examples you will reconsider this point. To give you another one:
>>
>>   __attribute__((nosync)) // or __attribute__((assume("nosync")) or #pragma omp assume ext_no_synchronization
>>   void user_code() {
>>     a = malloc();
>>     b = malloc();
>>     unknown_function_with_a_now_nosync_call_site(a, b);
>>     ...
>>     free(a);
>>   }
>
> This is a good example, that shows that if the user annotates a function with nosync then inferring nofree becomes easier. So the proposed semantics doesn't require nosync, but inference can be aided by its presence (not in this patch yet).

This is not at all what the example shows. The example shows that we cannot infer nofree for `unknown_function_with_a_now_nosync_call_site` with the new semantics but with the old semantics we could.
Only with the existing semantics you can optimize this example, not with the new one. I would have assumed this is implied given that I provided the example and I am arguing the proposed semantics are useless and prevent optimizations.

> I think all the reviewers' concerns have been addresses at this point. Please let us know if there's some remaining concern regarding the proposed semantics.

My concerns have not changed at all. Unsure why you would assume they have been addressed.

> The semantics proposed in this patch makes a lot of sense to me. And to make things clear, this proposal *does not* make nofree imply nosync (nor the other way around).

For deduction purposes it effectively does imply/require nosync. If you disagree, please provide an example where we can derive nofree without nosync under the proposed semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101701



More information about the llvm-commits mailing list