Page MenuHomePhabricator

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

Authored by fhahn on Tue, Jan 12, 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.Tue, Jan 12, 8:40 AM
fhahn requested review of this revision.Tue, Jan 12, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Jan 12, 8:40 AM
jdoerfert accepted this revision.Tue, Jan 12, 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.Tue, Jan 12, 8:48 AM
fhahn added inline comments.Tue, Jan 12, 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.Tue, Jan 12, 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.Tue, Jan 12, 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.Tue, Jan 12, 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.Tue, Jan 12, 9:25 AM

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

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

LGTM as well.

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

illreturn :)

This revision was landed with ongoing or failed builds.Tue, Jan 12, 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.