[PATCH] D77752: [llvm] Machine Learned policy for inlining -Oz

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/InliningAdvisor.cpp:32-34
+class InliningAdvisorImpl {};
+struct PendingInliningRecordImpl {};
+
----------------
dblaikie wrote:
> Doesn't look like these two definitions are required.
> 
> But more generally - this region of code seems to me like a lot of surface area to #def in and out for the ML parameters.
> 
> Does the non-trivial implementation of this pull in problematic dependencies (like LLVM's code ensures it works with or without zlib) or just a little extra insurance that this wouldn't adversely affect people when it's first committed? If it's the latter - I'd probably just have this be a runtime setting (so the functionality can be tested on every buildbot/developer, etc), not a compile time/#ifdef'd out situation.
> 
> 
> If it is a zlib-like situation, I'd still be in favor of a smaller surface area for the opt in/out - like having the basic interfaces in one library & then another library that implements the ML implementation - and a factory function that gives you either the trivial implementation or the ML one, etc.
> 
> 
> Ah, looking further on, I see this adds a TF dependency (aside: does this make LLVM & TF circularly dependent?) - yeah, then I'd expect the CMake variable would just be "has TF/doesn't have TF" and then maybe an LLVM library that requires TF/doesn't build/link without it (hmm, maybe that's more complicated than what you've got... I don't know for sure) - and then the usage would just be "x = #if TF new TFThing() #elif new NullThing #endif"\, etc.
If I understand it correctly, the idea would be to:

- detect optional dependencies (or be hinted where they are). 
- if any/all of them are available, compile dependent code
- use flags to pick the right implementation, if more than one is available. In extreme, if the compiler is compiled with both 'rel' and 'dev' modes, one would have 3 flag possibilities for the inliner: 'manual heuristic', 'ml-rel mode', and 'ml-dev mode'

I can see how this can result in cleaner code - probably all those ifdefs get removed, for example. 

I like it!


================
Comment at: llvm/lib/Analysis/ML/InliningModelFeatureMaps.h:34-54
+static const std::array<std::string, FeatureList::NumberOfFeatures>
+    FeatureNameMap{
+        /*FeatureList::CalleeBasicBlockCount*/ "callee_basic_block_count",
+        /*FeatureList::CallSiteHeight*/ "callsite_height",
+        /*FeatureList::NodeCount*/ "node_count",
+        /*FeatureList::NrCtantParams*/ "nr_ctant_params",
+        /*FeatureList::CostEstimate*/ "cost_estimate",
----------------
dblaikie wrote:
> static things in headers are problematic (since they define new/distinct variables in every translation unit that includes the header) - you can use inline accessor functions, or maybe constexpr variables... (I forget/don't know the specific linkage rules there) or declared in the header and defined in a cpp file
Ack - this sorts itself out with your earlier suggestion of building depending on availability of dependencies.


================
Comment at: llvm/lib/Analysis/ML/InliningModelRunnerProduction.h:56-63
+class FunctionSizeEstimator {
+public:
+  FunctionSizeEstimator(LLVMContext &Ctx, FunctionAnalysisManager &FAM) {}
+  // Just return the number of blocks. This is interesting for debugging only.
+  size_t getSizeEstimate(const Function &F) { return F.size(); }
+  bool isValid() const { return true; }
+};
----------------
dblaikie wrote:
> Ah, this is more of the "two implementations of the same entities" discussed earlier - yeah, I'd be in favor of avoiding this as it seems like it'll make the code quite a bit harder to work with (since it'll only compile in one mode at a time, so changes to these APIs will need different builds to validate that both implementatios are correctly updated, etc), concerns around testing, etc
Ack - addressing with the 'build everything if deps available'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77752