Page MenuHomePhabricator

Change TargetLibraryInfo analysis passes to always require Function
ClosedPublic

Authored by tejohnson on Mon, Aug 19, 10:14 AM.

Details

Summary

This is the first change to enable the TLI to be built per-function so
that -fno-builtin* handling can be migrated to use function attributes.
See discussion on D61634 for background. This is an enabler for fixing
handling of these options for LTO, for example.

This change should not affect behavior, as the provided function is not
yet used to build a specifically per-function TLI, but rather enables
that migration.

Most of the changes were very mechanical, e.g. passing a Function to the
legacy analysis pass's getTLI interface, or in Module level cases,
adding a callback. This is similar to the way the per-function TTI
analysis works.

There was one place where we were looking for builtins but not in the
context of a specific function. See FindCXAAtExit in
lib/Transforms/IPO/GlobalOpt.cpp. I'm somewhat concerned my workaround
could provide the wrong behavior in some corner cases. Suggestions
welcome.

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson created this revision.Mon, Aug 19, 10:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Aug 19, 10:14 AM

Thx for working on this Teresa.

lib/Analysis/GlobalsModRef.cpp
580 ↗(On Diff #215941)

Would auto& be more appropriate here? I'm concerned about a possible copy - we're taking the address of TLI on next line.
Same in the rest of the code.

lib/Transforms/IPO/GlobalOpt.cpp
2373 ↗(On Diff #215941)

Maybe explicitly write the nullptr

tejohnson marked 2 inline comments as done.Tue, Aug 20, 9:19 PM

Thanks for the comments. I'm out for a few days, but will address these when I get back.

lib/Analysis/GlobalsModRef.cpp
580 ↗(On Diff #215941)

It should be a cheap copy (TargetLibraryInfo only contains a pointer to its implementation), but yes we should be able to do this. I had run into issues earlier in another file due to the lifetime of a temporary returned by the lambda ending before the uses, but it looks like that can be addressed by changing the lambda there to have an explicit return type that is a reference, so that it doesn't return a temporary. I'll give that a try.

lib/Transforms/IPO/GlobalOpt.cpp
2373 ↗(On Diff #215941)

will do

tejohnson updated this revision to Diff 216946.Fri, Aug 23, 1:41 PM
tejohnson marked an inline comment as done.

Address comments

tejohnson marked an inline comment as done.Fri, Aug 23, 1:42 PM

@chandlerc @hfinkel can you take a look?

gchatelet marked an inline comment as done.Fri, Aug 23, 1:46 PM
gchatelet added inline comments.
lib/Analysis/GlobalsModRef.cpp
580 ↗(On Diff #215941)

Looks better now IMHO. Thx!

@chandlerc @hfinkel - can you take a look? Related to our discussion in D61634.

hfinkel accepted this revision.Thu, Sep 5, 1:13 PM

@chandlerc @hfinkel - can you take a look? Related to our discussion in D61634.

This LGTM.

This revision is now accepted and ready to land.Thu, Sep 5, 1:13 PM
This revision was automatically updated to reflect the committed changes.