This is an archive of the discontinued LLVM Phabricator instance.

[InlineCost] Introduce attributes to override InlineCost for inliner testing
ClosedPublic

Authored by DaniilSuchkov on Aug 31 2021, 5:00 PM.

Details

Summary

This patch introduces four new string attributes: function-inline-cost, function-inline-threshold, call-inline-cost and call-threshold-bonus.
These attributes allow you to selectively override some aspects of InlineCost analysis. That would allow us to test inliner separately from the InlineCost analysis.

That could be useful when you're trying to write tests for inliner and you need to test some very specific situation, like "the inline cost has to be this high", or "the threshold has to be this low". Right now every time someone does that, they have get creative to come up with a way to make the InlineCost give them the number they need (like adding ~30 load/add pairs for a trivial test). This process can be somewhat tedious which can discourage some people from writing enough tests for their changes. Also, that results in tests that are fragile and can be easily broken without anyone noticing it because the test writer can't explicitly control what input the inliner will get from the inline cost analysis.

These new attributes will alleviate those problems to an extent.
I decided to make them string attributes because enum attributes seem to me way too invasive for such a narrow use-case.

Diff Detail

Event Timeline

DaniilSuchkov created this revision.Aug 31 2021, 5:00 PM
DaniilSuchkov requested review of this revision.Aug 31 2021, 5:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 5:00 PM

this is a very nice feature :)

llvm/include/llvm/IR/InstrTypes.h
2282 ↗(On Diff #369818)

I'd rather not change the semantics of getFnAttr, can this be in InlineCost instead?

llvm/lib/Analysis/InlineCost.cpp
138

making this a template seems unnecessary, just use int? that's what all the callers are using anyway

DaniilSuchkov added inline comments.Sep 1 2021, 1:03 PM
llvm/include/llvm/IR/InstrTypes.h
2282 ↗(On Diff #369818)

This change makes getFnAttr's behavior similar to the one of hasFnAttr and hasRetAttr. Right now you can have a case when CB.getFnAttr(Attr).isValid() != CB.hasFnAttr(Attr) is true, which doesn't seem right, so I think that fixing it is better than creating a helper function that works around it.

However, I would agree that
a) what I described is a separate issue, definitely out of scope of this patch;
b) this change makes it way more likely that this patch will break something and will be reverted because of that
So a better decision is to add a workaround here, as you suggested, and then (maybe) fix this API inconsistency later in a separate patch.

llvm/lib/Analysis/InlineCost.cpp
138

Fair point.

Addressed the comments:

  1. Got rid of CallBase::getFnAttr change, moved that part into a local helper function.
  2. stringAttrAsInt isn't a template now.
DaniilSuchkov marked 2 inline comments as done.Sep 1 2021, 2:11 PM
mtrofin accepted this revision.Sep 1 2021, 4:27 PM

lgtm, some nits

llvm/lib/Analysis/InlineCost.cpp
165

why not move stringAttrAsInt here? has no other callers, and getStringFnAttrAsInt is just calling it.

This revision is now accepted and ready to land.Sep 1 2021, 4:27 PM

Merged stringAttrAsInt into getStringFnAttrAsInt.

DaniilSuchkov marked an inline comment as done.Sep 1 2021, 4:46 PM
This revision was landed with ongoing or failed builds.Sep 2 2021, 10:35 AM
This revision was automatically updated to reflect the committed changes.