[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:
> > 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.
> Maybe I should just pass the params directly, come to think of it.

If it's just two parameters, to one function/function call - yeah, doesn't seem like it's adding a lot of value.

Sometimes if a function ends up with too many arguments 

> 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.

Ah, thanks for explaining - callers would still be able to write CallSiteInfo{x, y} with a struct, if the 'CallSiteInfo' helps describe things. I'd tend to leave it up to the caller to decide what's most readable? (though I agree there are probably a few places in LLVM that get a bit over-enthusiastic about {}). Also, as it happens - even with the struct as-written, you could still write the less-legible version. You'd have to mark the ctor "explicit" to disallow bare {x, y} construction. (which would still allow CallSiteInfo{x, y}, FWIW).

> 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.

*nod* if it's only passed across the boundary into a function to group parameters, hopefully those scopes aren't too complicated/find where it's created/used/set/etc.

(admittedly, shorter functions helps here - big long functions risk code mutating parameters in ways that might be surprising, etc - so the benefit of using "const" for function parameters ("void f1(const int i) { /* really long body where it's nice to know 'i' isn't mutated */ }") but even that wouldn't need the struct to be intrinsically immutable)

There's some value, from my perspective, in having types act like basic (what some folks call "vocabulary") types as much as possible - C++ goes to quite some lengths to make it possible, and it reduces surprises/curiosities, at least I find, when types work that way. If I see a non-copyable type, for instance, I worry that someone's keeping pointers to it in some "interesting" way that means moving/copying it would be problematic.

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

Sure enough, but hopefully useful design discussions.


================
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;
----------------
mtrofin wrote:
> 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.
I think the inconsistency with the rest of the codebase would raise more questions than it'd answer - but it's sufficiently locally scoped it's not a huge deal. Mostly trying to discuss this not just in this case, but in the broader context of coding practices in LLVM in general.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77752