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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 2 inline comments as done.
mtrofin 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;
+};
----------------
dblaikie wrote:
> 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)
Maybe I should just pass the params directly, come to think of it.

What I mean re. readability is reading stuff like callingAFunction(param1, param2..., {value1, value2},...). If I read this code, I have no idea what that tuple means, I have to go to the definition and maybe implementation of callingAFunction. Instead, callingAFunction(..., CallSiteInfo(value1, value2)) adds a bit more info. It's not much extra, but it has no perf cost, so why not. The concern is also wrt what happens later, if the type is constructed in more places. 

Similarly, a naked struct would get me chasing more for "where is this field set". The getter-only design may help a reader understand this is meant to be a set-once value.

Anyway, if I just pass the scalar params, all this becomes moot.


================
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;
----------------
dblaikie wrote:
> 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.
I see value in const helping a reader understand where and when mutation may come. Here, it says "when constructed, then can't change". So it helps the reader see the whole object as one value, where parts don't shift.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77752