This is an archive of the discontinued LLVM Phabricator instance.

[CallGraph] Ignore callback uses
ClosedPublic

Authored by ggeorgakoudis on Jul 7 2020, 10:52 PM.

Details

Summary

Ignore callback uses when adding a callback function
in the CallGraph. Callback functions are typically
created when outlining, e.g. for OpenMP, so they have
internal scope and linkage. They should not be added
to the ExternalCallingNode since they are only callable
by the specified caller function at creation time.

A CGSCC pass, such as OpenMPOpt, may need to update
the CallGraph by adding a new outlined callback function.
Without ignoring callback uses, adding breaks CGSCC
pass restrictions and results to a broken CallGraph.

Diff Detail

Event Timeline

ggeorgakoudis created this revision.Jul 7 2020, 10:52 PM

I may be missing context, but this may be missing some wording.
*Why* should they not be in the callgraph?

ggeorgakoudis edited the summary of this revision. (Show Details)Jul 8 2020, 10:55 AM

I may be missing context, but this may be missing some wording.
*Why* should they not be in the callgraph?

Thanks for the comment. The callback function is in the callgraph but it is not added to the ExternalCallingNode. I have updated the commit message to include more information.

I may be missing context, but this may be missing some wording.
*Why* should they not be in the callgraph?

Thanks for the comment. The callback function is in the callgraph but it is not added to the ExternalCallingNode. I have updated the commit message to include more information.

Thanks

jdoerfert added inline comments.Jul 8 2020, 1:12 PM
llvm/include/llvm/IR/Function.h
834

Not: missing ..

llvm/lib/IR/Function.cpp
1489

Nit: missing ..

jdoerfert accepted this revision.Jul 8 2020, 1:36 PM

LGTM (forgot to accept earlier, sorry).

This revision is now accepted and ready to land.Jul 8 2020, 1:36 PM

Update for comment

ggeorgakoudis marked 2 inline comments as done.Jul 8 2020, 4:56 PM

Update regression test

This revision was automatically updated to reflect the committed changes.

I'm not sure if this is the commit to blame, but i think this might have broken http://45.33.8.238/linux/22501/step_12.txt

This revision is now accepted and ready to land.Jul 9 2020, 2:03 PM
lebedev.ri requested changes to this revision.Jul 9 2020, 2:09 PM

Yep, that was it.

This revision now requires changes to proceed.Jul 9 2020, 2:09 PM

Update regression tests under Transforms/Attributor/IPConstantProp

jdoerfert added inline comments.Jul 10 2020, 8:32 AM
llvm/lib/IR/Function.cpp
1499

You might need to inspect the ACS here. Check if it is really a callback callsite.

llvm/test/Transforms/Attributor/IPConstantProp/pthreads.ll
104 ↗(On Diff #277051)

TBH, this looks somehow we didn't run the script after a recent update. I'll commit an update to the tests.

jdoerfert added inline comments.Jul 10 2020, 2:00 PM
llvm/test/Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll
47 ↗(On Diff #277051)

The problem here is the missing "reference" edge in the old call graph. Before the patch bar is externally callable and foo calls an external function. With the patch, neither is the case but foo is also not calling bar (I think).

The solution is to verify we are looking at a callback call and not any abstract call site. Callback calls cause such "reference" edges since D82572.

Revert changes to regression tests

Check AbstractCallSite is indeed a callback

ggeorgakoudis marked an inline comment as done.Jul 11 2020, 2:57 AM
jdoerfert accepted this revision.Jul 12 2020, 3:01 PM
jdoerfert added inline comments.
llvm/lib/IR/Function.cpp
1499

Nit: Put the ACS stuff into a conditional guarded by the ignore. Trying to create an ACS is not completely free.

Update for latest nit comment

ggeorgakoudis marked an inline comment as done.Jul 14 2020, 8:49 AM
lebedev.ri resigned from this revision.Jul 14 2020, 9:49 AM

I don't actually have any opinion here, i just wanted to get the bots green (:

llvm/lib/Analysis/CallGraph.cpp
84

Why not just /*IgnoreCallbackUses=*/true?

This revision is now accepted and ready to land.Jul 14 2020, 9:49 AM

Update for latest comment

ggeorgakoudis marked an inline comment as done.Jul 14 2020, 11:17 AM
This revision was automatically updated to reflect the committed changes.