This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Record references to aliases to calls
ClosedPublic

Authored by tejohnson on Oct 7 2016, 4:15 PM.

Details

Summary

When there is a call to an alias in the same module, we were not
adding either a reference or call edge. So we could incorrectly think
that the alias was dead if it was inlined in that function, despite
having a reference imported elsewhere. This resulted in unsats at
link time.

Add a reference edge when the call is to an alias, since we won't
add a call edge for it.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 74000.Oct 7 2016, 4:15 PM
tejohnson retitled this revision from to [ThinLTO] Record references to aliases to calls.
tejohnson updated this object.
tejohnson added reviewers: davide, mehdi_amini.
tejohnson added a subscriber: llvm-commits.
davide accepted this revision.Oct 7 2016, 4:33 PM
davide edited edge metadata.

LGTM. Thanks Teresa!

This revision is now accepted and ready to land.Oct 7 2016, 4:33 PM
mehdi_amini added inline comments.Oct 7 2016, 4:39 PM
lib/Analysis/ModuleSummaryAnalysis.cpp
58 ↗(On Diff #74000)

I don't understand this change. I seem to me that you're adding a reference when we're calling an alias. Why isn't the alias added as a callee?

tejohnson added inline comments.Oct 7 2016, 5:16 PM
lib/Analysis/ModuleSummaryAnalysis.cpp
58 ↗(On Diff #74000)

Because there is no called function (it's null). Note we can still find the aliasee function in the ref graph via the alias summary. Maybe we should add a call edge to the alias regardless, so we could import via the alias summary...let me try that .

mehdi_amini added inline comments.Oct 7 2016, 5:28 PM
lib/Analysis/ModuleSummaryAnalysis.cpp
58 ↗(On Diff #74000)

I have a doubt about what we generate (and should generate) on you example, is it:

  • main reference analias
  • main calls aliasee
  • analias reference aliasee

Or:

  • main calls analias
  • analias reference aliasee

I tend to lean for the second one.

tejohnson added inline comments.Oct 7 2016, 10:30 PM
lib/Analysis/ModuleSummaryAnalysis.cpp
58 ↗(On Diff #74000)

Aliases are represented explicitly in the summary, although they are not created here.

What we will generate with my new fix is the same as what we are already generating if the call to the alias was intra-module. This is simply:

  • main calls analias

Later on we will generate an ALIAS record in the summary to record the relationship:

  • analias aliases aliasee

(in the bitcode writer)

Then in the bitcode reader we will turn that into an AliasSummary so that we can follow the call from main to analias (recorded in the FunctionSummary for main) through to aliasee via the AliasSummmary.

tejohnson updated this revision to Diff 74018.Oct 7 2016, 10:31 PM
tejohnson edited edge metadata.

Create a call edge to the alias instead of a ref edge.

tejohnson added inline comments.Oct 7 2016, 10:33 PM
lib/Analysis/ModuleSummaryAnalysis.cpp
58 ↗(On Diff #74000)

Aliases are represented explicitly in the summary, although they are not created here.

Meaning the "edge" (AliasSummary) is not created here

What we will generate with my new fix is the same as what we are already generating if the call to the alias was intra-module.

I meant inter-module.

mehdi_amini added inline comments.Oct 7 2016, 10:36 PM
lib/Analysis/ModuleSummaryAnalysis.cpp
116 ↗(On Diff #74018)

What if we have an alias to an unnamed function?

tejohnson added inline comments.Oct 7 2016, 10:42 PM
lib/Analysis/ModuleSummaryAnalysis.cpp
116 ↗(On Diff #74018)

I thought these would all get named in the NameAnonGlobals pass?

In fact, perhaps this should be an assert that we have a name?

mehdi_amini accepted this revision.Oct 7 2016, 10:47 PM
mehdi_amini edited edge metadata.

LGTM now!

Thanks.

lib/Analysis/ModuleSummaryAnalysis.cpp
116 ↗(On Diff #74018)

Oh right, I forgot about that.

This revision was automatically updated to reflect the committed changes.