[PATCH] D87489: [NFC][MLInliner] Presort instruction successions.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

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.



================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:90-92
+    return std::is_sorted(
+        FunctionFeatures::ImportantInstructionSuccessions.begin(),
+        FunctionFeatures::ImportantInstructionSuccessions.end());
----------------
Could replace this with `llvm::is_sorted(FunctionFeatures::ImportantInstructionSuccessions);` while you're here


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:105
+const std::array<std::pair<size_t, size_t>, 137>
+    IRToNativeSizeLearning::FunctionFeatures::ImportantInstructionSuccessions{
+        {{1, 1},   {1, 4},   {1, 5},   {1, 7},   {1, 8},   {1, 9},   {1, 11},
----------------
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?)


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:141-143
   for (auto &BB : F)
     for (auto &I : BB)
       Ret += TTI.getInstructionCost(
----------------
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)


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:176-177
 
   auto StartID = 0;
   auto LastID = StartID;
   auto getPairIndex = [](size_t a, size_t b) {
----------------
'int' would be appropriate here - 'auto' isn't helping readability, it's longer than 'int'


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:180-182
         std::find(FunctionFeatures::ImportantInstructionSuccessions.begin(),
                   FunctionFeatures::ImportantInstructionSuccessions.end(),
                   std::make_pair(a, b));
----------------
Could simplify this a bit with `llvm::find(FunctionFeatures::ImportantInstructionSuccessions, std::make_pair(a, b))` - separate commit, doesn't need precommit review, etc.


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:191-193
     for (auto I = BB.instructionsWithoutDebug().begin(),
               E = BB.instructionsWithoutDebug().end();
          I != E; ++I) {
----------------
range-based-for loop, perhaps?


================
Comment at: llvm/lib/Analysis/InlineSizeEstimatorAnalysis.cpp:280-286
 InlineSizeEstimatorAnalysis::InlineSizeEstimatorAnalysis() {}
 InlineSizeEstimatorAnalysis ::InlineSizeEstimatorAnalysis(
     InlineSizeEstimatorAnalysis &&) {}
 InlineSizeEstimatorAnalysis::~InlineSizeEstimatorAnalysis() {}
 InlineSizeEstimatorAnalysis::Result
 InlineSizeEstimatorAnalysis::run(const Function &F,
                                  FunctionAnalysisManager &FAM) {
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87489/new/

https://reviews.llvm.org/D87489