c++: consider built-in operator candidates first

  During operator overload resolution, we currently consider non-member
candidates before built-in candidates.  This didn't make a difference
before r12-3346, but after this change add_candidates will avoid
computing excess argument conversions if we've already seen a strictly
viable candidate, so it's better to consider built-in candidates first.

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk?

gcc/cp/ChangeLog:

	* call.c (add_operator_candidates): Consider built-in operator
	candidates before considering non-member candidates.

gcc/testsuite/ChangeLog:

	* g++.dg/template/conv17.C: Extend test.
---
 gcc/cp/call.c                          | 13 +++++++------
 gcc/testsuite/g++.dg/template/conv17.C |  7 +++++++
 2 files changed, 14 insertions(+), 6 deletions(-)
  

On 9/20/21 12:46, Patrick Palka wrote:
> During operator overload resolution, we currently consider non-member
> candidates before built-in candidates.  This didn't make a difference
> before r12-3346, but after this change add_candidates will avoid
> computing excess argument conversions if we've already seen a strictly
> viable candidate, so it's better to consider built-in candidates first.

Doesn't r12-3346 stop considering conversions after it sees a bad one, 
and later return to the bad candidate if there is no strictly viable 
candidate?  How does this patch change that?

Depending on the order of the candidates seems fragile.

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk?
> 
> gcc/cp/ChangeLog:
> 
> 	* call.c (add_operator_candidates): Consider built-in operator
> 	candidates before considering non-member candidates.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/template/conv17.C: Extend test.
> ---
>   gcc/cp/call.c                          | 13 +++++++------
>   gcc/testsuite/g++.dg/template/conv17.C |  7 +++++++
>   2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index c5601d96ab8..c0da083758f 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -6321,7 +6321,6 @@ add_operator_candidates (z_candidate **candidates,
>   			 vec<tree, va_gc> *arglist,
>   			 int flags, tsubst_flags_t complain)
>   {
> -  z_candidate *start_candidates = *candidates;
>     bool ismodop = code2 != ERROR_MARK;
>     tree fnname = ovl_op_identifier (ismodop, ismodop ? code2 : code);
>   
> @@ -6333,6 +6332,12 @@ add_operator_candidates (z_candidate **candidates,
>     if (rewritten && code != EQ_EXPR && code != SPACESHIP_EXPR)
>       flags &= ~LOOKUP_REWRITTEN;
>   
> +  /* Add built-in candidates to the candidate set.  The standard says to
> +     rewrite built-in candidates, too, but there's no point.  */
> +  if (!rewritten)
> +    add_builtin_candidates (candidates, code, code2, fnname, arglist,
> +			    flags, complain);
> +
>     bool memonly = false;
>     switch (code)
>       {
> @@ -6352,6 +6357,7 @@ add_operator_candidates (z_candidate **candidates,
>   
>     /* Add namespace-scope operators to the list of functions to
>        consider.  */
> +  z_candidate *start_candidates = *candidates;
>     if (!memonly)
>       {
>         tree fns = lookup_name (fnname, LOOK_where::BLOCK_NAMESPACE);
> @@ -6423,11 +6429,6 @@ add_operator_candidates (z_candidate **candidates,
>   
>     if (!rewritten)
>       {
> -      /* The standard says to rewrite built-in candidates, too,
> -	 but there's no point.  */
> -      add_builtin_candidates (candidates, code, code2, fnname, arglist,
> -			      flags, complain);
> -
>         /* Maybe add C++20 rewritten comparison candidates.  */
>         tree_code rewrite_code = ERROR_MARK;
>         if (cxx_dialect >= cxx20
> diff --git a/gcc/testsuite/g++.dg/template/conv17.C b/gcc/testsuite/g++.dg/template/conv17.C
> index f0f10f2ef4f..87ecefb8de3 100644
> --- a/gcc/testsuite/g++.dg/template/conv17.C
> +++ b/gcc/testsuite/g++.dg/template/conv17.C
> @@ -61,3 +61,10 @@ concept E = requires { T().h(nullptr); };
>   
>   static_assert(!E<C>);
>   #endif
> +
> +// Verify that the strictly viable built-in operator+ candidate precludes
> +// us from computing all argument conversions for the below non-strictly
> +// viable non-member candidate.
> +enum N { n };
> +int operator+(N&, B);
> +int f = n + 42;
>
  

On Mon, 20 Sep 2021, Jason Merrill wrote:

> On 9/20/21 12:46, Patrick Palka wrote:
> > During operator overload resolution, we currently consider non-member
> > candidates before built-in candidates.  This didn't make a difference
> > before r12-3346, but after this change add_candidates will avoid
> > computing excess argument conversions if we've already seen a strictly
> > viable candidate, so it's better to consider built-in candidates first.
> 
> Doesn't r12-3346 stop considering conversions after it sees a bad one, and
> later return to the bad candidate if there is no strictly viable candidate?
> How does this patch change that?

Yes, but add_candidates also looks for a strictly viable candidate among
the already-considered candidates in the 'candidates' list via the line:

  bool seen_strictly_viable = any_strictly_viable (*candidates);

So by considering the built-in candidates first, the subsequent call to
add_candidates that considers the non-member functions in will be aware
of any (built-in) strictly viable candidate.

> 
> Depending on the order of the candidates seems fragile.

Yeah.. :/  I guess in general it'd be better to build up the entire
overload set first and then call add_candidates exactly once (which
would also make the perfect candidate optimization more consistent/effective).
But I'm not sure if we can easily build up such an overload set in this
case since built-in candidates are represented and handled differently
than non-built-in candidates..

FWIW, although the test case added by this patch is contrived, this
opportunity was found in the real world by instrumenting the 'bad_fns'
mechanism added by r12-3346 to look for situations where we still end up
using it (and thus end up redundantly considering some candidates twice),
and this built-in operator situation was the most common in the
codebases that I tested (although still quite rare in the codebases that
I tested).

> 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk?
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* call.c (add_operator_candidates): Consider built-in operator
> > 	candidates before considering non-member candidates.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/template/conv17.C: Extend test.
> > ---
> >   gcc/cp/call.c                          | 13 +++++++------
> >   gcc/testsuite/g++.dg/template/conv17.C |  7 +++++++
> >   2 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> > index c5601d96ab8..c0da083758f 100644
> > --- a/gcc/cp/call.c
> > +++ b/gcc/cp/call.c
> > @@ -6321,7 +6321,6 @@ add_operator_candidates (z_candidate **candidates,
> >   			 vec<tree, va_gc> *arglist,
> >   			 int flags, tsubst_flags_t complain)
> >   {
> > -  z_candidate *start_candidates = *candidates;
> >     bool ismodop = code2 != ERROR_MARK;
> >     tree fnname = ovl_op_identifier (ismodop, ismodop ? code2 : code);
> >   @@ -6333,6 +6332,12 @@ add_operator_candidates (z_candidate **candidates,
> >     if (rewritten && code != EQ_EXPR && code != SPACESHIP_EXPR)
> >       flags &= ~LOOKUP_REWRITTEN;
> >   +  /* Add built-in candidates to the candidate set.  The standard says to
> > +     rewrite built-in candidates, too, but there's no point.  */
> > +  if (!rewritten)
> > +    add_builtin_candidates (candidates, code, code2, fnname, arglist,
> > +			    flags, complain);
> > +
> >     bool memonly = false;
> >     switch (code)
> >       {
> > @@ -6352,6 +6357,7 @@ add_operator_candidates (z_candidate **candidates,
> >       /* Add namespace-scope operators to the list of functions to
> >        consider.  */
> > +  z_candidate *start_candidates = *candidates;
> >     if (!memonly)
> >       {
> >         tree fns = lookup_name (fnname, LOOK_where::BLOCK_NAMESPACE);
> > @@ -6423,11 +6429,6 @@ add_operator_candidates (z_candidate **candidates,
> >       if (!rewritten)
> >       {
> > -      /* The standard says to rewrite built-in candidates, too,
> > -	 but there's no point.  */
> > -      add_builtin_candidates (candidates, code, code2, fnname, arglist,
> > -			      flags, complain);
> > -
> >         /* Maybe add C++20 rewritten comparison candidates.  */
> >         tree_code rewrite_code = ERROR_MARK;
> >         if (cxx_dialect >= cxx20
> > diff --git a/gcc/testsuite/g++.dg/template/conv17.C
> > b/gcc/testsuite/g++.dg/template/conv17.C
> > index f0f10f2ef4f..87ecefb8de3 100644
> > --- a/gcc/testsuite/g++.dg/template/conv17.C
> > +++ b/gcc/testsuite/g++.dg/template/conv17.C
> > @@ -61,3 +61,10 @@ concept E = requires { T().h(nullptr); };
> >     static_assert(!E<C>);
> >   #endif
> > +
> > +// Verify that the strictly viable built-in operator+ candidate precludes
> > +// us from computing all argument conversions for the below non-strictly
> > +// viable non-member candidate.
> > +enum N { n };
> > +int operator+(N&, B);
> > +int f = n + 42;
> > 
> 
>
  

On 9/20/21 15:32, Patrick Palka wrote:
> On Mon, 20 Sep 2021, Jason Merrill wrote:
> 
>> On 9/20/21 12:46, Patrick Palka wrote:
>>> During operator overload resolution, we currently consider non-member
>>> candidates before built-in candidates.  This didn't make a difference
>>> before r12-3346, but after this change add_candidates will avoid
>>> computing excess argument conversions if we've already seen a strictly
>>> viable candidate, so it's better to consider built-in candidates first.
>>
>> Doesn't r12-3346 stop considering conversions after it sees a bad one, and
>> later return to the bad candidate if there is no strictly viable candidate?
>> How does this patch change that?
> 
> Yes, but add_candidates also looks for a strictly viable candidate among
> the already-considered candidates in the 'candidates' list via the line:
> 
>    bool seen_strictly_viable = any_strictly_viable (*candidates);
> 
> So by considering the built-in candidates first, the subsequent call to
> add_candidates that considers the non-member functions in will be aware
> of any (built-in) strictly viable candidate.

Ah, I get it, the problem is that the first add_candidates can't see any 
strictly-viable candidates.

>> Depending on the order of the candidates seems fragile.
> 
> Yeah.. :/  I guess in general it'd be better to build up the entire
> overload set first and then call add_candidates exactly once (which
> would also make the perfect candidate optimization more consistent/effective).
> But I'm not sure if we can easily build up such an overload set in this
> case since built-in candidates are represented and handled differently
> than non-built-in candidates..

Or as another way of getting the same effect, add another possible value 
of shortcut_bad_convs to mean leave bad candidates incomplete in the 
candidates list, and then once we're done adding candidates and still 
don't have a viable one, we can go back and finish processing the bad 
candidates?

Either way, this could also help when there are both member and 
non-member candidates for the operator.

> FWIW, although the test case added by this patch is contrived, this
> opportunity was found in the real world by instrumenting the 'bad_fns'
> mechanism added by r12-3346 to look for situations where we still end up
> using it (and thus end up redundantly considering some candidates twice),
> and this built-in operator situation was the most common in the
> codebases that I tested (although still quite rare in the codebases that
> I tested).

>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
>>> trunk?
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* call.c (add_operator_candidates): Consider built-in operator
>>> 	candidates before considering non-member candidates.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/template/conv17.C: Extend test.
>>> ---
>>>    gcc/cp/call.c                          | 13 +++++++------
>>>    gcc/testsuite/g++.dg/template/conv17.C |  7 +++++++
>>>    2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
>>> index c5601d96ab8..c0da083758f 100644
>>> --- a/gcc/cp/call.c
>>> +++ b/gcc/cp/call.c
>>> @@ -6321,7 +6321,6 @@ add_operator_candidates (z_candidate **candidates,
>>>    			 vec<tree, va_gc> *arglist,
>>>    			 int flags, tsubst_flags_t complain)
>>>    {
>>> -  z_candidate *start_candidates = *candidates;
>>>      bool ismodop = code2 != ERROR_MARK;
>>>      tree fnname = ovl_op_identifier (ismodop, ismodop ? code2 : code);
>>>    @@ -6333,6 +6332,12 @@ add_operator_candidates (z_candidate **candidates,
>>>      if (rewritten && code != EQ_EXPR && code != SPACESHIP_EXPR)
>>>        flags &= ~LOOKUP_REWRITTEN;
>>>    +  /* Add built-in candidates to the candidate set.  The standard says to
>>> +     rewrite built-in candidates, too, but there's no point.  */
>>> +  if (!rewritten)
>>> +    add_builtin_candidates (candidates, code, code2, fnname, arglist,
>>> +			    flags, complain);
>>> +
>>>      bool memonly = false;
>>>      switch (code)
>>>        {
>>> @@ -6352,6 +6357,7 @@ add_operator_candidates (z_candidate **candidates,
>>>        /* Add namespace-scope operators to the list of functions to
>>>         consider.  */
>>> +  z_candidate *start_candidates = *candidates;
>>>      if (!memonly)
>>>        {
>>>          tree fns = lookup_name (fnname, LOOK_where::BLOCK_NAMESPACE);
>>> @@ -6423,11 +6429,6 @@ add_operator_candidates (z_candidate **candidates,
>>>        if (!rewritten)
>>>        {
>>> -      /* The standard says to rewrite built-in candidates, too,
>>> -	 but there's no point.  */
>>> -      add_builtin_candidates (candidates, code, code2, fnname, arglist,
>>> -			      flags, complain);
>>> -
>>>          /* Maybe add C++20 rewritten comparison candidates.  */
>>>          tree_code rewrite_code = ERROR_MARK;
>>>          if (cxx_dialect >= cxx20
>>> diff --git a/gcc/testsuite/g++.dg/template/conv17.C
>>> b/gcc/testsuite/g++.dg/template/conv17.C
>>> index f0f10f2ef4f..87ecefb8de3 100644
>>> --- a/gcc/testsuite/g++.dg/template/conv17.C
>>> +++ b/gcc/testsuite/g++.dg/template/conv17.C
>>> @@ -61,3 +61,10 @@ concept E = requires { T().h(nullptr); };
>>>      static_assert(!E<C>);
>>>    #endif
>>> +
>>> +// Verify that the strictly viable built-in operator+ candidate precludes
>>> +// us from computing all argument conversions for the below non-strictly
>>> +// viable non-member candidate.
>>> +enum N { n };
>>> +int operator+(N&, B);
>>> +int f = n + 42;
>>>
>>
>>
>