This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Look through indirect calls
ClosedPublic

Authored by jdoerfert on Oct 21 2021, 7:12 PM.

Details

Summary

Through the new Attributor::checkForAllCallees we can look through
indirect calls and visit all potential callees if they are known.

Diff Detail

Event Timeline

jdoerfert created this revision.Oct 21 2021, 7:12 PM
jdoerfert requested review of this revision.Oct 21 2021, 7:12 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: wdng. · View Herald Transcript
arsenm added inline comments.Oct 21 2021, 7:22 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1749

Why the compare to 1 for a bool?

jdoerfert added inline comments.Oct 21 2021, 9:37 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1749

If we have more than 1 potential callee we don't know if it is a "must" callee anymore. I haven't thought enough about this to be sure we need or don't need it but there is the possibility we want to distinguish callees that may be called here from the one that certainly is.

ormris removed a subscriber: ormris.Jan 24 2022, 11:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2022, 8:42 AM

ping, what is the status of this?

jdoerfert updated this revision to Diff 554869.Aug 30 2023, 5:19 PM
jdoerfert retitled this revision from [WIP][Attributor] Look through indirect calls to [Attributor] Look through indirect calls.
jdoerfert edited the summary of this revision. (Show Details)
jdoerfert marked an inline comment as done.Aug 31 2023, 6:49 PM

ping, what is the status of this?

Ready for review :)

tianshilei1992 added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
1740

/*NumCallees=*/ 1?

1749

Probably instead of using NumCallees (I'm not sure if the CB will need the value), can we use something like bool IsUnique?

jdoerfert added inline comments.Sep 1 2023, 11:32 AM
llvm/lib/Transforms/IPO/Attributor.cpp
1740

will do.

1749

I figured it doesn't cost anything to provide more information. Once can use it for a heuristic, so decide not to spend time if the #callees is larget han some threshold.

tianshilei1992 added inline comments.Sep 1 2023, 11:48 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
3306

I thought the optimistic condition would be we don't have indirect calls such that the "callee" is required.

llvm/lib/Transforms/IPO/Attributor.cpp
1749

I kinda think the interface of the CB just looks a little weird. Essentially the best way is something like ArrayRef<Function *> Callees, and then we can have both information. However, checkForAll* interfaces itself mean doing the check one by one instead of passing an array to the CB.
That being said, I don't have a strong objection to it, but just prefer a clean definition. If in the future we will need the number, we can revise it.

jdoerfert added inline comments.Sep 1 2023, 2:06 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
3306

It's not about optimistic per se. This is a way to avoid creating AAs that will result in a pessimistic fixpoint right away. With this change, most AAs use checkForAllCallees via AACalleeToCallSite and therefore will be able to cope with "missing"/"non-unique" callees.

jdoerfert updated this revision to Diff 555546.Sep 1 2023, 7:36 PM
This revision was not accepted when it landed; it landed in state Needs Review.Sep 8 2023, 12:14 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.