Re: [PATCH] decide edge's hotness when there is profile info

[Jan Hubicka <hubicka at ucw dot cz>]

> I have a warning like that already in drop_profile(). Is that

I think it should be warning (or silent) for COMDAT and error/note for
other functions (depending on flag_profile_correction).
I guess drop_profile is better place for it indeed.

> sufficient? Also, Steven Bosscher suggested putting that warning under
> OPT_Wdisabled_optimization instead of '0', what do you prefer for
> that?

It is a bit different case than other disabled optimizations we have (where
the optimization does not happen because of --param tunable limits), but I
am fine with both alternatives.
The warning may end up quite noisy so having way to silence it definitely
makes sense.
> 
> >> +    }
> >> +
> >> +  /* Propagate the profile dropping to other 0-count COMDATs that are
> >> +     potentially called by COMDATs we already dropped the profile on.  */
> >> +  while (worklist.length () > 0)
> >> +    {
> >> +      struct cgraph_edge *e;
> >> +
> >> +      node = worklist.pop ();
> >> +      for (e = node->callees; e; e = e->next_caller)
> >> +        {
> >> +          struct cgraph_node *callee = e->callee;
> >> +          struct function *fn = DECL_STRUCT_FUNCTION (callee->decl);
> >> +
> >> +          if (callee->count > 0)
> >> +            continue;
> >> +          if (DECL_COMDAT (callee->decl) && fn && fn->cfg
> >> +              && profile_status_for_function (fn) == PROFILE_READ)
> >
> > Perhaps we can check here maybe_hot_bb_p for the caller and rely on profile estimate
> > to give us false only for really known to be unlikely paths? (i.e. EH handling, noreturns
> > etc.)
> 
> Ok, let me try this.
> 
> >> +            {
> >> +              /* Since there are no non-0 call counts to this function,
> >> +                 we don't know for sure whether it is hot. Indicate to
> >> +                 the drop_profile routine that function should be marked
> >> +                 normal, rather than hot.  */
> >> +              drop_profile (node, false);
> >> +              worklist.safe_push (callee);
> >> +            }
> >> +        }
> >> +    }
> >> +  worklist.release ();
> >
> > I would add a pointer set so we avoid duplicates in worklist.  This is potentially quadratic
> > for large programs.
> 
> I'll add a visited_nodes set to keep track of processed nodes so they
> don't get added to the worklist multiple times.

Perhaps you can also track this by testing profile_status_for_function. Both solutions are fine for me.

Honza
> 
> Thanks,
> Teresa
> 
> >
> > OK, with these changes.
> >
> > Honza
> 
> 
> 
> -- 
> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413