On Thu, Nov 14, 2024 at 07:07:48PM +0100, Jan Hubicka wrote:
> > Hi!
> >
> > The inlining heuristics uses DECL_DECLARED_INLINE_P (whether a function
> > has been explicitly marked inline; that can be inline keyword, or for C++
> > also constexpr keyword or defining a function inside of a class definition)
> > heavily to increase desirability of inlining a function etc.
> > In most cases it is desirable, people usually mark functions inline with
> > the intent that they are actually inlined.
>
> Reading the discussion on the PR, one of argument for adding new
> attribute was that we should not make differnece between "constexr" and
> "constexpr inline" since C++ stnadard says it is the same.
>
> We already do such difference between
>
> class foo
> {
> int bar () { }
> inline int bar2 () { }
> };
There is just small difference between those two.
DECL_DECLARED_INLINE_P is set for both of those (and when using constexpr
keyword as well).
For bar2 in grokfndecl:
10993 /* If the declaration was declared inline, mark it as such. */
10994 if (inlinep)
10995 {
10996 DECL_DECLARED_INLINE_P (decl) = 1;
10997 if (publicp)
10998 DECL_COMDAT (decl) = 1;
10999 }
11000 if (inlinep & 2)
11001 DECL_DECLARED_CONSTEXPR_P (decl) = true;
(which also shows that it is set also for constexpr explicit keyword).
And for bar in grokmethod:
19161 /* p1779 ABI-Isolation makes inline not a default for in-class
19162 definitions attached to a named module. If the user explicitly
19163 made it inline, grokdeclarator will already have done the right
19164 things. */
19165 if ((!named_module_attach_p ()
19166 || flag_module_implicit_inline
19167 /* Lambda's operator function remains inline. */
19168 || LAMBDA_TYPE_P (DECL_CONTEXT (fndecl)))
19169 /* If the user explicitly asked for this to be inline, we don't
19170 need to do more, but more importantly we want to warn if we
19171 can't inline it. */
19172 && !DECL_DECLARED_INLINE_P (fndecl))
19173 {
19174 if (TREE_PUBLIC (fndecl))
19175 DECL_COMDAT (fndecl) = 1;
19176 DECL_DECLARED_INLINE_P (fndecl) = 1;
19177 /* It's ok if we can't inline this. */
19178 DECL_NO_INLINE_WARNING_P (fndecl) = 1;
19179 }
The only difference is DECL_NO_INLINE_WARNING_P set on the ones without
explicit inline, i.e. -Winline doesn't warn unless there is explicit inline
or constexpr keyword.
Anyway, the point is that most of the code in the wild will just use
constexpr keyword, even if it means to inline it, because the standard says
it implies inline and because the normal C++ coding style also uses just
that. The standard itself certainly doesn't use that (constexpr inline or
inline constexpr makes a difference just to variables).
> We have 10 bits left, but it would also make sense to put the flag into
> inline summary and use explicit lookup_attribute elsewhere. Inliner is
> only place that cares about this in hot code.
>
> IPA parts of the patch looks OK however I wonder if simply clearing
> DECL_DECLARED_INLINE_P would have same effect?
If it is in ipa_fn_summary, where would I do the lookup_attribute?
Anyway, looking at the spots where I've used DECL_OPTIMIZABLE_INLINE_P
(agree it is a bad name), I think tree_inlinable_function_p
and expand_call_inline don't have access to it (so would need to
lookup_attribute it), devirtualization_time_bonus does,
estimate_size_and_time too, ipa-icf.cc doesn't (I think), ipa-inline.cc
does though I think the code will be less readable when using those,
ipa-split.cc I doubt it has them.
So, I think it would be easier to keep it at least for now as a flag
in function_decl and perhaps add a comment that if we run out of bits,
this one could be changed into ipa_fn_summary + some lookup_attributes.
And DECL_AGGRESSIVE_INLINING_P looks reasonable.
Jakub
