Re: [RFC PATCH v2] c++: extend cold, hot attributes to classes

[Jason Merrill <jason@redhat.com>]

On 8/10/23 14:06, Javier Martinez wrote:
> 
> Thanks for the comments, Jason.
> v2: + Fix formatting, remove unnecessarily warning.
> 
> On Tue, Aug 8, 2023 at 10:28 PM Jason Merrill <jason@redhat.com 
> <mailto:jason@redhat.com>> wrote:
>  > Seems reasonable, but how do you expect this to be used?
> We have large applications where some large classes are known to be used 
> only at startup. Marking every member function as cold is impractical so 
> we would mark those classes as cold.

Sure, though I think of the cold attribute as being primarily to mark 
unlikely paths in otherwise hot code; is this a

if (!initialized)
   do_initialization();

kind of situation?

>  > I think doing this here will miss lazily-declared special member
>  > functions; I wonder if you want to copy the attribute in grokclassfn
>  > instead?
> I think they are being handled correctly with the current patch. 
> Considered the following program:
> 
> class __attribute((cold)) Foo {
> public:
>      int m_x, m_y;
>      auto operator<=>(const Foo&) const = default;
> };
> 
> int main(void) {
>      Foo a{1,1};
>      Foo b{1,2};
> 
>      std::set<Foo> s; // OK
>      s.insert(a);     // OK
> 
>      std::cout << std::boolalpha
>          << (a == b) << ' '
>          << (a != b) << ' '
>          << (a <  b) << ' '
>          << (a <= b) << ' '
>          << (a >  b) << ' '
>          << (a >= b) << ' '
>          << std::endl;
> }
> 
> Per the rules for any operator<=> overload, a defaulted <=> overload 
> will also allow the type to be compared with <, <=, >, and >=. If 
> operator<=> is defaulted and operator== is not declared at all, then 
> operator== is implicitly defaulted.
> The GIMPLE dump shows:
>  > __attribute__((cold))
>  > struct strong_ordering Foo::operator<=> (const struct Foo * const 
> this, const struct Foo & D.64195)
>  > __attribute__((cold))
>  > bool Foo::operator== (const struct Foo * const this, const struct Foo 
> & D.64206)
> 
> I think this makes sense as add_implicitly_declared_members is called 
> before my injection via finish_struct_1 -> check_bases_and_members.

Yes, but that's because the implicit op== isn't declared lazily like 
some other special member functions (CLASSTYPE_LAZY_*/lazily_declare_fn) 
which can happen after the class is complete.

> ---
> 
> I would like some comment on the implications of:
> -  { "cold",       0, 0, true,  false, false, false,
> +  { "cold",      0, 0, false,  false, false, false,
> 
> As I am now introducing a new warning that I cannot explain in an old test:
> testsuite/g++.dg/Wmissing-attributes.C:55:22: warning: 'hot' attribute 
> ignored [-Wattributes]
> 
>  > template <>
>  > void*
>  > ATTR ((hot)) ATTR ((alloc_size (1))) ATTR ((malloc))
>  > missing_all<char>(int);   // T = char, same as above

I think this is because attributes in that position appertain to the 
return type rather than the declaration.  At attribs.cc:753, if an 
attribute has decl_required true and we try to apply it to a return 
type, we pass it along to the declaration instead; if you change the 
true to false we will try to apply it to void* directly and fail.

I think it would work to check for (flags & (ATTR_FLAG_FUNCTION_NEXT | 
ATTR_FLAG_DECL_NEXT)) and return without warning in that case.  You'd 
still set *no_add_attr.

BTW the patch got garbled this time, probably safer to make it an 
attachment if you're using gmail.

Jason