In http://reviews.llvm.org/D21405, Rong is adding a pre-inliner pass which uses the SimpleInliner pass with lower thresholds. If that is added, the use of -inline[.*]-threshold options will affect the preinliner as well which is not the intended behavior. This patch moves the threshold flags out of InlineCost.cpp and passes a set of knobs (based on the flags) to the getInlineCost method. This would allow the pre-inliner to ignore the threshold related flags.
Details
Diff Detail
Event Timeline
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
115 | Ok, will rename this. | |
lib/Transforms/IPO/InlineSimple.cpp | ||
91 | This is the current behavior : see the code in updateThreshold. It also makes sense. If the tool user sees a flag and explicitly passes a value to it, they expect the value to take effect. Even if you disagree with this, that should be a separate change. |
lib/Transforms/IPO/InlineSimple.cpp | ||
---|---|---|
91 | OK -- if this is intended as NFC. On the other hand how do you plan to handle fine grain control of different instances of the inliner ? (LTO, thinLTO, IRPGO)? |
lib/Transforms/IPO/InlineSimple.cpp | ||
---|---|---|
91 | If we want their thresholds to be controlled by a flag, we need a separate flags and create separate entry points. For example, we should add a createPrePnliner pass that should set the threshold using a -preinline-threshold flag. |
Well, in that case, you need to create a separate entry point that doesn't override the passed threshold. The behavior of createFunctionInliningPass should remain unchanged. Consider -O3 for example. What should happen if you pass -O3 and -inline-threshold=1000 ?
You
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
115 | The renaming to Parameters has not being done. | |
117 | Add brief documentation on each params | |
169–170 | Should defaultThreshold be wrapped in inline-params too? Edit: probably not. The defaultThreshold here means many different things (opt level specific default when default-threshold option is not specified, threshold passed in API, or indirect call threshold etc.) -- I hope this can be cleaned up at some point: cost analysis should just look at the InlineParams to make decisions. |
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
169–170 | I actually think it should be moved to InlineParams. It is computed by different means, but the meaning is the same - use this threshold in the absence of any other overrides. If you agree I'll move this as well. |
lgtm with the nit suggestion.
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
118 | I prefer this one be called 'InlineThreshold' (to not be confused with the internal option) and document how it is set. |
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
118 | Or perhaps keeping this parameter name but changing the option name to be InlineThreshold (not Default...) |
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
38 | Clang driver maps O4 to O3 (so O4 is actually 'supported'). |
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
38 | Sure, but that's a clang "compatibility option" (O4 use to trigger LTO on top of O3). On the LLVM side O4 does not exist (I'm not sure it has ever existed in LLVM). |
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
38 | ok -- this should be cleaned up then (it was in the original code that got moved here). By the way, why deprecating O4? It is common to use it to turn on LTO. |
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
38 | I don't know exactly, all I could find was: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20130819/185217.html |
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
38 | Because LTO requires a complete change to the compilation model -- it isn't just an optimization level. Still further, the other optimization levels still make sense within that model. We might want -Oz, -Os, -O2, and -O3 very usefully (and with different results) when doing LTO. I would keep O4 out of LLVM for clarity -- it is really only supposed to be a compatibility thing in Clang. We should probably mark it as deprecated or something as I switched the behavior a loooong time ago. | |
118 | I like calling it "default" if it is OptSize is and such are going to override it. Which parameter were you thinking that is currently named default? | |
lib/Transforms/IPO/InlineSimple.cpp | ||
34–48 | I would much prefer these command line flags remain in the InlineCost analysis and next to all the other numbers used to tune things. |
To clarify, this patch itself is just a NFC refactoring in preparation for the real change, right? (to prevent the existing cl::opt flags from interfering with preinline for instrumentation)
A couple nits inline.
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
122 | Can you add a comment about what happens when the "Optional" is in its "empty" state? I assume that DefaultThreshold is used in their place but it would be good to document explicitly what happens. | |
lib/Analysis/InlineCost.cpp | ||
613 | This doesn't match the LLVM naming convention for functions (should start with lower case): http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly Also, you may want to make this 'min' function a local lambda in updateThreshold as it is only used there I think. |
Yes. I will add a new interface to create a SimpleInliner instance for pre-inlining which will ignore the -inline.*-threshold
flags while creating the InlineParams object
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
38 | I have changed the comment to "Use when -O3 is specified". | |
118 | I have changed the command line flag's variable name to InlineThreshold (in sync with the "inline-threshold" string) | |
122 | I've expanded the comments. Let me know if you want to add anything else specific. | |
lib/Analysis/InlineCost.cpp | ||
613 | Made this into a lambda and renamed it to minIfPresent | |
lib/Transforms/IPO/InlineSimple.cpp | ||
34–48 | I am a bit unsure what you were suggesting but I assume you want to move this to InlineCost.cpp as globals in llvm namespace and reference them in InlineSimple.cpp |
Tried to be more clear below. If this still isn't making sense, maybe let's talk or use IRC?
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
121 | nit: I find it much easier to read with vertical whitespace between the values here. | |
lib/Analysis/InlineCost.cpp | ||
623 | nit: We typically name local lambdas following the variable naming conventions even though they can be called. | |
lib/Transforms/IPO/InlineSimple.cpp | ||
72–74 | Sorry, I guess I wasn't clear before... I'm specifically suggesting keeping all of this logic inside of InlineCost.cpp as how the *default* parameters get set. If we need the ability to get custom parameters which still reflect the command line flags, there should be some API in InlineCost.h that the inliner queries to get the necessary params. My goal here stems from two related principles:
A minor secondary goal -- the inliner shouldn't have to do extra work to get the defaults. Those should just be used. It is when an inliner pass wants to deviate from the defaults that it should take explicit steps and call explicit flags. |
lib/Transforms/IPO/InlineSimple.cpp | ||
---|---|---|
72–74 | Thanks for the clarification. I have moved getInlineParams and other associated logic that computes the threshold from the flags to InlineCost.cpp. SimpleInliner now calls one of these methods to get an InlineParams object which gets passed to getInlineCost. |
lib/Transforms/IPO/InlineSimple.cpp | ||
---|---|---|
72–74 | Why can't getInlineCost do this by-default? |
lib/Transforms/IPO/InlineSimple.cpp | ||
---|---|---|
72–74 | Summarizing an in-person chat with Easwaran: The reason is because the createInlineFunctionsPass thing below actually takes the optimization level directly as an input. Or an integer threshold. So we need to actually pass that information along to get the correct inline params. I suggested sinking the call to getInlineParams all the way down to that create routine so that it was completely separate from the actual pass construction. Not ideal, but not a lot of good options here. Thanks for explaining this Easwaran, I was having trouble connecting the dots. =] |
Chandler, I have not moved the logic in getInlineParams with no arguments to InlineParams constructor since leaving the default to be a trivial constructor is preferable when the pre-inliner creates a custom InlineParams object.
LGTM with a bunch of nits addressed below. Thanks for bearing with me on the layering or structure of the code here, and thanks for the nice cleanup!
include/llvm/Analysis/InlineCost.h | ||
---|---|---|
144 | nit: s/used/is used/ | |
149 | You might want to document what OptLevel and SizeOptLevel can be here. We've kind-of failed to do that for a long time sadly, but I think you can at least document the requirements that the internal code places upon them even if we want to incrementally improve this later. | |
171–174 | nit: I would put the Params parameter in the same position as DefaultThreshold was previously here and below. | |
lib/Analysis/InlineCost.cpp | ||
959–961 | As you're not calling a constructor I would use = for initializing. Also, auto seems appropriate here. So: auto IndirectCallParams = Params; | |
1541–1546 | This seems like it might be a better fit merged into the comments for the InlineParams class? Certainly it seems like documentation for users rather than implementation documentation. |
lib/Analysis/InlineCost.cpp | ||
---|---|---|
1541–1546 | This also helped me catch some stale comments about InlineParams! |
There is no O4, did you mean O{1,2,3} here?