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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D77752#1990087 <https://reviews.llvm.org/D77752#1990087>, @mtrofin wrote:

> Incorporated feedback to implement the 2 modes as buildable depending on presence of dependencies, meaning they may both be present.


Hey - thanks for having a go at what I was describing, but this isn't /quite/ what I was thinking and still involves quite a few different bits of macro usage across different situations (some conditional functionality implemented in the build files, some of it in source files - the two different implementations (trivial/null/empty and non-trivial) #ifdef'd out, etc)

At a high level:

- I think the preprocessor macros provided by the build system should be named after the libraries that have been detected, not the functionality those libraries are intended to provide (rather than LLVM_USE_ML_POLICY_DEV, it'd be something more like ZLIB (LLVM_ENABLE_TF, LLVM_ENABLE_TF_LITE or something?))
- I think it'd be clearer if all the conditional was done in the source/header files, not mixed between that and the build files.
- If possible, I'd like to avoid having two different implementations of multiple classes like that - what I'd picture was one factory function that says "get the thing" and that implementation would check the runtime parameter from the user and call unconditionally provided (but conditionally implemented) functions that retrieve implementations of an interface. There would be a default/null implementation, if needed (if it's easier to have that than to have the callers check for presence/absence) - a separate class, rather than a #ifdef implementation of another class.



================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:47-48
+  CallSiteInfo(CallBase *CB, unsigned H) : Call(CB), Height(H) {}
+  CallBase *const Call;
+  const unsigned Height;
+};
----------------
LLVM doesn't usually use top-level const like this, FWIW - I'd suggest avoiding it since it does things like disable assignability, etc.

(also you don't necessarily have to write that ctor - you can use braced init to construct such a thing memberwise without having a ctor written)


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:117
+    for (auto *F : DeletedFunctions)
+      delete (F);
+  }
----------------
Drop the `()` around `F` here (instead `delete F;`).

But perhaps more generally: could `DeletedFunctions` contain `unique_ptr`s, so there's less manual memory management here? (instead this function could be `DeletedFunctions.clear();`)


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:469
+size_t InliningAdvisorImpl::getSizeEstimate(const Function &F) const {
+  return NativeSizeEstimates.getOrInsert(&F, [this](const Function *F) {
+    return SizeEstimator->getSizeEstimate(*F);
----------------
Generally I'd encourage using `[&]` for captures in any lambda that doesn't escape its scope, doubly so if it doesn't escape the statement it's defined in.


================
Comment at: llvm/lib/Analysis/ML/Rel/InliningModelRunnerRel.cpp:24-32
+namespace llvm {
+
+static const char *const FeedPrefix = "feed_";
+static const char *const FetchPrefix = "fetch_";
+
+/// InliningModelRunner - production mode implementation. It uses a AOT-compiled
+/// SavedModel for efficient execution.
----------------
Externally visible definitions should go in headers (unless this is a definition of a pimpl or the like) - otherwise this type should be defined in an anonymous namespace (so it can't collide with other implementation details in other files)

https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77752