[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 {};
+
----------------
mtrofin wrote:
> 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!
Updated along these lines. 


================
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;
+};
----------------
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.


================
Comment at: llvm/lib/Analysis/ML/InliningAdvisor.cpp:117
+    for (auto *F : DeletedFunctions)
+      delete (F);
+  }
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77752