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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
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;
+};
----------------
mtrofin wrote:
> dblaikie wrote:
> > 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)
> Removed the const and changed to explicit readonly accessors. I do want to underscore this is an immutable object - easier to read / maintain. If it's OK style-wise, I prefer the explicit ctor for readability, too.
But it's not really immutable (well, even less now, without const members) since it can be reassigned (OldCSI = CallSiteInfo(NewCB, NewH))

& what's the readability benefit you have in mind in terms of having a ctor? 

It looks like this is only constructed in one place, and everything else refers to it via const ref - so I'm not sure it'd present a significant loss of readability if it were a plain struct?

(this isn't a "you must not do it this way", but a bit of a discussion to understand if this/things like this are worthwhile compared to many other use cases that have simple structs, etc)


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:117
+    for (auto *F : DeletedFunctions)
+      delete (F);
+  }
----------------
mtrofin wrote:
> dblaikie wrote:
> > 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();`)
> I need to do lookups in the DeletedFunctions set. That would require some gymnastics with std::unique_ptr around find, I think. Not sure it's worth the complexity in this case - is there an alternative?
Ah, right. Yeah - maybe leave a FIXME to fix this once we're using C++20, which supports heterogenous lookup.

Or might be worth using SmallPtrSet (https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallptrset-h) here anyway (which doesn't & probably can't support unique_ptr elements anyway, just an unfortunate reality) for reduced memory usage/better cache locality.


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:136
+    Memoized() = default;
+    Memoized(const Memoized &) = delete;
+    Memoized(Memoized &&) = default;
----------------
This'll be implicitly deleted, FWIW (similarly the assignment operators will be implicitly deleted or omitted as necessary)


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:173-178
+    const std::string CallerName;
+    const std::string CalleeName;
+    const InliningFeatures Features;
+    const bool SiteWasInlined;
+    const int NativeDeltaSize;
+    const bool Mandatory;
----------------
again, top level const like this is a bit quirky/uncommon in LLVM, but not outright banned

Even if the struct isn't mutable - your vector is, and the equivalent to mutation could be achieved by removing an element from the vector and adding a new one with the desired values, so make const members aren't really providing the invariants you'd like them to.


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:209-217
+  InliningAdvisorImpl *const Advisor;
+  const Function *const Caller;
+  const Function *const Callee;
+  const size_t EstimatedNativeSizeCallerBefore;
+  const size_t EstimatedNativeSizeCalleeBefore;
+  const bool Mandatory;
+  const int CallerIRSize;
----------------
Similarly - not sure if making these members const carries its weight - essentially it's to ensure these members aren't modified during "recordInlining"? (I don't see any other code that could be accessing these? But then I guess I wonder why aren't they private if they're only accessed by that member? Perhaps I'm missing something)


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:272-274
+  if (!Mandatory) {
+    Advisor->ModelRunner->receiveReward(NativeDeltaSize);
+  }
----------------
LLVM usually skips braces on single line (some people go so far as to omit braces on single statement (even if that statement has to be wrapped over several lines)) blocks - and several single line blocks in this code skip braces - so this one (& maybe others in this patch? I haven't looked) seems inconsistent.


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:465
+      Ret.Name = F->getName().str();
+    if (DeletedFunctions.count(F) > 0) {
+      assert(getNrOfUsers(*F) == 0);
----------------
This is often written as just "if (x.count(y))" but up to you. (about 30:1 ratio in favor of that compared to > 0 or != 0)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77752