This is an archive of the discontinued LLVM Phabricator instance.

[NFC][MLInliner] Presort instruction successions.
ClosedPublic

Authored by mtrofin on Sep 10 2020, 8:34 PM.

Diff Detail

Event Timeline

mtrofin created this revision.Sep 10 2020, 8:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 10 2020, 8:34 PM
mtrofin requested review of this revision.Sep 10 2020, 8:34 PM
dblaikie accepted this revision.Sep 10 2020, 8:54 PM

Looks good - thanks!

Few other bits of optional feedback, I don't mean to dredge up old code that I know we discussed somewhat in the past - but some thoughts in case any resonate/make more sense now than in the past, and some minor bits of stuff that can be cleaned up without pre-commit review.

llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp
90–92

Could replace this with llvm::is_sorted(FunctionFeatures::ImportantInstructionSuccessions); while you're here

105

This could be a static variable here ^ rather than being a member variable - would save all the IRToNativeSizeLearning::FunctionFeatures:: qualification - doesn't look like that qualification adds a lot of value to the variable name? (I tend to think "longer variable names for wider scopes" - given the effective scope of this variable is just two functions here - maybe doesn't need such a long name (at least the qualifiers, "ImportantInstructionSuccessions" is probably sufficiently unambiguous?)

141–143

Throughout the file there are a fair few non-const references that could be const, which would be preferred. (generally top level const on local values is less interesting - but usually if a reference can be const, it should be made so)

176–177

'int' would be appropriate here - 'auto' isn't helping readability, it's longer than 'int'

180–182

Could simplify this a bit with llvm::find(FunctionFeatures::ImportantInstructionSuccessions, std::make_pair(a, b)) - separate commit, doesn't need precommit review, etc.

191–193

range-based-for loop, perhaps?

280–286

Hmm - I know when went around a few times on the API a while ago - and perhaps there's good reasons I've since forgotten (sorry :/) but this sort of if/else is something I was hoping to avoid. (by, eg: Having a virtual interface, a TF implementation and a non-TF stub implementation (or fallbacks in the calling code that tested for the absence of an implementation - "if null do the stub/default thing"), and then one function with the #if/else that - and then a single always-compiled function that has a conditional #include and then a "get estimator" function that returns null/default, or the TF implementation if available) - having lots of surface area like this means more chance of failures when someone's building only in one mode or the other. Specifically having different implementations of the same function I think is something best avoided where possible (LLVM has some in the platform APIs, for instance - open a file for Windows, open a file for Linux, etc - but in this case I hope it could be simpler by having only one very small amount of preprocessor conditionalized implementation)

This revision is now accepted and ready to land.Sep 10 2020, 8:54 PM
mtrofin added inline comments.Sep 10 2020, 9:21 PM
llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp
280–286

We did that for the big things - the inline advisor. Here, there's not much of a surface area, and seemed more pragmatic to do it this way.

mtrofin updated this revision to Diff 291124.Sep 10 2020, 9:40 PM
mtrofin marked 7 inline comments as done.

feedback

This revision was landed with ongoing or failed builds.Sep 10 2020, 9:41 PM
This revision was automatically updated to reflect the committed changes.
mtrofin added inline comments.Sep 10 2020, 9:41 PM
llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp
141–143

yup - thanks, fixed.