This is an archive of the discontinued LLVM Phabricator instance.

[FunctionAttrs] Derive willreturn for fns with readonly` & `mustprogress`.
ClosedPublic

Authored by fhahn on Jan 12 2021, 8:40 AM.

Details

Summary

Similar to D94125, derive willreturn for functions that are readonly and
mustprogress in FunctionAttrs.

To quote the reasoning from D94125:

Since D86233 we have `mustprogress` which, in combination with
`readonly`, implies `willreturn`. The idea is that every side-effect
has to be modeled as a "write". Consequently, `readonly` means there
is no side-effect, and `mustprogress` guarantees that we cannot "loop"
forever without side-effect.

Diff Detail

Event Timeline

fhahn created this revision.Jan 12 2021, 8:40 AM
fhahn requested review of this revision.Jan 12 2021, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2021, 8:40 AM
jdoerfert accepted this revision.Jan 12 2021, 8:48 AM

I left some comments that need to be addressed, not necessarily how I proposed it but somehow. LGTM otherwise.

llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1434

|| F->hasFnAttr(Attribute::WillReturn)

1437

Changed = true; ;)

This revision is now accepted and ready to land.Jan 12 2021, 8:48 AM
fhahn added inline comments.Jan 12 2021, 8:54 AM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1437

Changed = true; ;)

yeah.

I guess a comment here might be helpful as well. It is possible that the function has been marked as noreturn earlier, e.g. because it does not have any returns. In such cases, will return seems a better choice, if possible.

nikic added inline comments.Jan 12 2021, 8:58 AM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1437

Is it possible to keep both, or does that cause verifier errors? A function that always unwinds should be able to be both noreturn and willreturn without invoking UB.

jdoerfert added inline comments.Jan 12 2021, 9:01 AM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1437

Good point. Both seem sensible, assuming exit and abort are considered "returning". I'd argue they unwind the stack like an exception (that is potentially not captured) so we should be good.

fhahn added inline comments.Jan 12 2021, 9:24 AM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1437

Sounds good to me, I'll update the code to not drop it; I don't think there's a verifier for this. The only thing is that at first glance it might be surprising to have a function both willreturn and noreturn

fhahn updated this revision to Diff 316129.Jan 12 2021, 9:25 AM

Address comments, also add NumWillReturn stats counter and Function::willReturn()/Function::setWillReturn.

nikic accepted this revision.Jan 12 2021, 9:29 AM

LGTM as well.

llvm/test/Transforms/FunctionAttrs/willreturn.ll
4–5

illreturn :)

This revision was landed with ongoing or failed builds.Jan 12 2021, 12:03 PM
This revision was automatically updated to reflect the committed changes.

Thanks for taking a look!

llvm/test/Transforms/FunctionAttrs/willreturn.ll
4–5

thanks, uploaded too soon, fixed in committed version.