Page MenuHomePhabricator

Change TargetLibraryInfo analysis passes to always require Function

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



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

Diff Detail


Event Timeline

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

Thx for working on this Teresa.

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.

2373 ↗(On Diff #215941)

Maybe explicitly write the nullptr

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

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

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.

2373 ↗(On Diff #215941)

will do

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

Address comments

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

@chandlerc @hfinkel can you take a look?

gchatelet marked an inline comment as done.Aug 23 2019, 1:46 PM
gchatelet added inline comments.
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.Sep 5 2019, 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.Sep 5 2019, 1:13 PM
This revision was automatically updated to reflect the committed changes.