Page MenuHomePhabricator

[Analyzer] Add -analyzer-config option for function size the inliner considers as large
ClosedPublic

Authored by seaneveson on Aug 27 2015, 7:35 AM.

Details

Summary

Dear All,

I would like to propose a small patch to add an option (-analyzer-config min-blocks-for-inline-large=14) to control the function size the inliner considers as large, in relation to "max-times-inline-large". In my patch the option defaults to the original hard coded behaviour, which I believe should be adjustable with the other inlining settings.

The analyzer-config test has been modified so that the analyzer will reach the getMinBlocksForInlineLarge() method and store the result in the ConfigTable, to ensure it is dumped by the debug checker.

Regards,

Sean Eveson
SN Systems - Sony Computer Entertainment Group

Diff Detail

Repository
rL LLVM

Event Timeline

seaneveson updated this revision to Diff 33325.Aug 27 2015, 7:35 AM
seaneveson retitled this revision from to [Analyzer] Add -analyzer-config option for function size the inliner considers as large.
seaneveson updated this object.
seaneveson added a subscriber: cfe-commits.

This looks fine, but I'm not a fan of the actual name chosen here. It's not very clear what it means by just looking at the name, and as we grow the number of configuration options that will become more important.

A few suggestions:

  • min-block-size-treat-functions-as-large
  • min-cfg-size-treat-functions-as-large (since "blocks" is a bit ambiguous)
  • min-cfg-size-categorize-functions-as-large

Unrelated to this patch, several of our other options suffer from my same concerns, e.g.: max-times-inline-large, max-inclinable-size, but changing those are not the focus of this patch.

Other than the name, this looks good to me. Whatever name we choose, I think the method name should also be appropriately named, e.g.:

  • getMinCFGSizeToTreatFunctionsAsLarge()
seaneveson updated this revision to Diff 34196.Sep 8 2015, 1:15 AM

I changed the name to min-cfg-size-treat-functions-as-large, thanks for the suggestion.

If the patch is acceptable can someone commit it for me?

dcoughlin accepted this revision.Sep 10 2015, 5:20 PM
dcoughlin edited edge metadata.

LGTM. I will commit. Thanks Sean!

This revision is now accepted and ready to land.Sep 10 2015, 5:20 PM
This revision was automatically updated to reflect the committed changes.

LGTM as well. Thanks Sean.