This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Derive `willreturn` based on `mustprogress`
ClosedPublic

Authored by jdoerfert on Jan 5 2021, 3:09 PM.

Details

Summary

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

jdoerfert created this revision.Jan 5 2021, 3:09 PM
jdoerfert requested review of this revision.Jan 5 2021, 3:09 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: bbn. · View Herald Transcript
jdoerfert added inline comments.Jan 6 2021, 6:58 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2352

I think I can even remove this condition.

fhahn added inline comments.Jan 11 2021, 7:20 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2350

Do we have to also ensure that the function is nounwind? In case of an exception, we would return to the closest exception handler and not return to a point in the existing call stack.

jdoerfert added inline comments.Jan 11 2021, 7:26 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2350

No, they are by design orthogonal. Willreturn "allows" to "come back through unwind. Basically, you will go back and unwind through the call of the function, that means you "return" to the call. So willreturn does not imply nounwind and willreturn does not guarantee there is no exception either.
Here is the lang ref:

This function attribute indicates that a call of this function will either exhibit undefined behavior or comes back and continues execution at a point in the existing call stack that includes the current invocation. Annotated functions may still raise an exception, i.a., nounwind is not implied. If an invocation of an annotated function does not return control back to a point in the call stack, the behavior is undefined.

fhahn added a comment.Jan 12 2021, 8:40 AM

The logic in general seems fine to me, but I'm not too familiar with the attributor internals, so it would be good for someone more familiar to take a look.

BTW, I put up a similar patch for -function-attrs D94502

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
2350

Ah right, that makes sense. Not sure, but IMO the current wording could be a bit improved, but I am not really sure how.

2352

I assume the MemAA.*ReadOnly helpers also return true if the function does not access memory? Then it would probably be simpler to just drop the check.

fhahn accepted this revision.Feb 17 2021, 12:28 PM

LGTM thanks! It would probably be good to wait a bit with committing in case someone else more familiar with the attributor internals wants to chime in.

This revision is now accepted and ready to land.Feb 17 2021, 12:28 PM