This is an archive of the discontinued LLVM Phabricator instance.

[deref] No need to check nosync in addition to nofree
AbandonedPublic

Authored by mkazantsev on Apr 15 2021, 5:07 AM.

Details

Summary

The attribute nofree guarantees that a function cannot free memory
by any means, directly or indirectly. If it is specified, there is no need to
check nosync separately because it imposes extra limitations (for example,
a function can sync but never write or free memory).

This was already discussed in D99135, but the discussion doesn't explain
case of synching readonly/nofree function. Even if nosync is implied by
something else, we still should not rely on this attribute being explicitly
propagated before we came there.

Diff Detail

Event Timeline

mkazantsev created this revision.Apr 15 2021, 5:07 AM
mkazantsev requested review of this revision.Apr 15 2021, 5:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 5:07 AM
mkazantsev edited the summary of this revision. (Show Details)Apr 15 2021, 5:10 AM
reames requested changes to this revision.Apr 16 2021, 11:26 AM

Change rejected. the nofree function attribute does explicitly allow freeing by another thread. See e.g. discussion here: https://reviews.llvm.org/D100141#2695360

I am really curious how you stumbled into this? There's only one currently enabled case which uses this codepath, and it has an explicit check for the GC abstract machine model which should cover your use case as I understand it?

This revision now requires changes to proceed.Apr 16 2021, 11:26 AM
mkazantsev added a comment.EditedApr 18 2021, 9:29 PM

Change rejected. the nofree function attribute does explicitly allow freeing by another thread. See e.g. discussion here: https://reviews.llvm.org/D100141#2695360

I am really curious how you stumbled into this? There's only one currently enabled case which uses this codepath, and it has an explicit check for the GC abstract machine model which should cover your use case as I understand it?

I am planning to use this checker for loop load PRE. In case of concurrent GC, this check is too restrictive because nosync is never inferrable in practice.

mkazantsev abandoned this revision.Apr 19 2021, 8:32 AM

I am really curious how you stumbled into this? There's only one currently enabled case which uses this codepath, and it has an explicit check for the GC abstract machine model which should cover your use case as I understand it?

I am planning to use this checker for loop load PRE. In case of concurrent GC, this check is too restrictive because nosync is never inferrable in practice.

Max, it might be worth talking offline about the specifics of your case.

In the abstract machine model, nosync should be inferrable in many cases. In the abstract machine model, you also don't need to do, and there's an existing check in code. (Which you might need to extend downstream.)