Page MenuHomePhabricator

[FuncAttrs] Add willreturn to readonly callsites in mustprogress fns.
Needs ReviewPublic

Authored by fhahn on Feb 18 2021, 4:25 AM.

Details

Summary

willreturn is now required by a number of passes in order to remove
function calls. Currently we fail to eliminate readonly function calls
in mustprogress functions, because we do not combine the mustprogress
info of the containing function with the readonly info at call sites.

This can lead to situations where calls that are guaranteed to return
are not eliminated from C++ code, as discussed on llvm-dev.
(https://lists.llvm.org/pipermail/llvm-dev/2021-February/148596.html)

This patch updates FunctionAttrs to add willreturn to readonly call
sites in mustprogress functions or willreturn function. This also allows
adding willreturn to the containing function, if all function calls can
be marked as willreturn.

Markgin callsites in a willreturn function as willreturn, can also be
helpful in the presence of inlining. There is a potentialcyclic dependency
between adding willreturn to functions and callsites in a function:
adding willreturn to callsites first enables adding willreturn to the
function in some cases. But adding willreturn to a function first
enables adding willreturn to more callsites.

For now, add it to callsites first.

Diff Detail

Event Timeline

fhahn created this revision.Feb 18 2021, 4:25 AM
fhahn requested review of this revision.Feb 18 2021, 4:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 18 2021, 4:25 AM
uenoku added a subscriber: uenoku.Feb 18 2021, 4:48 AM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1508

Changed should be orred here.

jdoerfert accepted this revision.Feb 18 2021, 8:30 AM
jdoerfert added inline comments.
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1449

The comment is confusing. If the caller is willreturn the call site behavior doesn't matter. If the caller is "only" mustprogress the call site behavior matters.

1464

You could track if any call was not willreturn to avoid doing so in functionWillReturn.

This revision is now accepted and ready to land.Feb 18 2021, 8:30 AM
jdoerfert requested changes to this revision.Feb 18 2021, 8:31 AM

accidentally accepted, I think the logic is fine. I keep it under review in case you decide to do more than the two nits and reuse the traversal result. If not, the logic looks OK to me

This revision now requires changes to proceed.Feb 18 2021, 8:31 AM
nikic added a comment.Feb 18 2021, 8:54 AM

After D96960 I don't think this change is really necessary anymore (at least it does not need an LLVM 12 backport). If the language has forward progress, then all function definitions will get annotated anyway, so only extern declarations are relevant. However, those will not be readyonly either -- unless they are explicitly annotated such, in which case they will also be annotated as willreturn after D96960.

(I do think that doing callsite attribute inference is *generally* valuable, the comment here is more along the lines that it does not seem necessary to implement this as a quick fix for just willreturn to prevent regressions.)

After D96960 I don't think this change is really necessary anymore (at least it does not need an LLVM 12 backport). If the language has forward progress, then all function definitions will get annotated anyway, so only extern declarations are relevant. However, those will not be readyonly either -- unless they are explicitly annotated such, in which case they will also be annotated as willreturn after D96960.

I am not sure if this is completely true. When looking at the test changes for D96960, I have the impression that a lot of __builtin_xxx functions might also need the 'willreturn' attribute. (and for 'C', it should be clang that adds the attribute).

fhahn updated this revision to Diff 324674.Feb 18 2021, 9:15 AM

addressed comments.

After D96960 I don't think this change is really necessary anymore (at least it does not need an LLVM 12 backport). If the language has forward progress, then all function definitions will get annotated anyway, so only extern declarations are relevant. However, those will not be readyonly either -- unless they are explicitly annotated such, in which case they will also be annotated as willreturn after D96960.

I am not sure if this is completely true. When looking at the test changes for D96960, I have the impression that a lot of __builtin_xxx functions might also need the 'willreturn' attribute. (and for 'C', it should be clang that adds the attribute).

For C++ only, D96960 is probably sufficient.

But I think it should still be helpful when doing LTO with mixed C/C++ code (a C function definition can be determined to be readonly and will be willreturn when called in a C++ function). And there are other languages with different ways of marking functions as readonly.

FWIW, having call site deduction is generally helpful, also for willreturn. I'd much rather spend time on a lightweight Attributor pass than this though, there it is part of the overall model anyway.

fhahn marked an inline comment as done.Feb 18 2021, 9:17 AM
fhahn added inline comments.
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1449

updated

1464

I updated the code but due to the early exit I think we need to track the possibility that we did not analyze the call sites, so unfortunately it's a bit more complicated.

jdoerfert added inline comments.Feb 18 2021, 9:21 AM
llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1464

Yeah, I see that now. The design here was not meant to couple those things. Unsure if this is better :(

1468
nikic added a comment.Feb 18 2021, 9:33 AM

I am not sure if this is completely true. When looking at the test changes for D96960, I have the impression that a lot of __builtin_xxx functions might also need the 'willreturn' attribute. (and for 'C', it should be clang that adds the attribute).

That may well be, but in that case we really need to get the attribute directly on the __builtin functions, otherwise the issue would persist for C.

Do you have any particular examples? I would expect that most builtins get lowered to intrinsics and those should already have all the necessary attributes, but there's probably some cases that fell through.

Do you have any particular examples? I would expect that most builtins get lowered to intrinsics and those should already have all the necessary attributes, but there's probably some cases that fell through.

I am not able to figure out a useful one, based on the list in clang/test/CodeGen/complex-builtins.c and clang/test/CodeGen/complex-libcalls.c.
I do see expected optimization to happen, but at the same time I am not seeing that they get annotated with 'willreturn'.