[pushed] c++: fix tourney logic

[pushed] c++: fix tourney logic

[Jason Merrill]

Tested x86_64-pc-linux-gnu, applying to trunk.  Patrick, sorry I didn't apply
this sooner.

-- 8< --

In r13-3766 I changed the logic at the end of tourney to avoid redundant
comparisons, but the change also meant skipping any less-good matches
between the champ_compared_to_predecessor candidate and champ itself.

This should not be a correctness issue, since we believe that joust is a
partial order.  But it can lead to missed warnings, as in this testcase.

gcc/cp/ChangeLog:

	* call.cc (tourney): Only skip champ_compared_to_predecessor.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wsign-promo1.C: New test.
---
 gcc/cp/call.cc                           |  5 +++--
 gcc/testsuite/g++.dg/warn/Wsign-promo1.C | 15 +++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/warn/Wsign-promo1.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 657eca93d23..a49fde949d5 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13227,10 +13227,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
      been compared to.  */
 
   for (challenger = candidates;
-       challenger != champ
-	 && challenger != champ_compared_to_predecessor;
+       challenger != champ;
        challenger = challenger->next)
     {
+      if (challenger == champ_compared_to_predecessor)
+	continue;
       fate = joust (champ, challenger, 0, complain);
       if (fate != 1)
 	return NULL;
diff --git a/gcc/testsuite/g++.dg/warn/Wsign-promo1.C b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
new file mode 100644
index 00000000000..51b76eee735
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
@@ -0,0 +1,15 @@
+// Check that we get joust warnings from comparing the final champ to a
+// candidate between it and the previous champ.
+
+// { dg-additional-options -Wsign-promo }
+
+struct A { A(int); };
+
+enum E { e };
+
+int f(int, A);
+int f(unsigned, A);
+int f(int, int);
+
+int i = f(e, 42);		// { dg-warning "passing 'E'" }
+// { dg-warning "in call to 'int f" "" { target *-*-* } .-1 }

base-commit: 084addf8a700fab9222d4127ab8524920d0ca481
-- 
2.39.3

Re: [pushed] c++: fix tourney logic

[Patrick Palka]

On Fri, 20 Oct 2023, Jason Merrill wrote:

> Tested x86_64-pc-linux-gnu, applying to trunk.  Patrick, sorry I didn't apply
> this sooner.
> 
> -- 8< --
> 
> In r13-3766 I changed the logic at the end of tourney to avoid redundant
> comparisons, but the change also meant skipping any less-good matches
> between the champ_compared_to_predecessor candidate and champ itself.
> 
> This should not be a correctness issue, since we believe that joust is a
> partial order.  But it can lead to missed warnings, as in this testcase.

I suppose this rules out optimizing tourney via transitivity when in
a non-SFINAE context since it'd cause missed warnings such as these.
But maybe we'd still want to optimize the second pass via transitivity
in a SFINAE context?

> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (tourney): Only skip champ_compared_to_predecessor.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/warn/Wsign-promo1.C: New test.
> ---
>  gcc/cp/call.cc                           |  5 +++--
>  gcc/testsuite/g++.dg/warn/Wsign-promo1.C | 15 +++++++++++++++
>  2 files changed, 18 insertions(+), 2 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index 657eca93d23..a49fde949d5 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -13227,10 +13227,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
>       been compared to.  */
>  
>    for (challenger = candidates;
> -       challenger != champ
> -	 && challenger != champ_compared_to_predecessor;
> +       challenger != champ;
>         challenger = challenger->next)
>      {
> +      if (challenger == champ_compared_to_predecessor)
> +	continue;
>        fate = joust (champ, challenger, 0, complain);
>        if (fate != 1)
>  	return NULL;
> diff --git a/gcc/testsuite/g++.dg/warn/Wsign-promo1.C b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> new file mode 100644
> index 00000000000..51b76eee735
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> @@ -0,0 +1,15 @@
> +// Check that we get joust warnings from comparing the final champ to a
> +// candidate between it and the previous champ.
> +
> +// { dg-additional-options -Wsign-promo }
> +
> +struct A { A(int); };
> +
> +enum E { e };
> +
> +int f(int, A);
> +int f(unsigned, A);
> +int f(int, int);
> +
> +int i = f(e, 42);		// { dg-warning "passing 'E'" }
> +// { dg-warning "in call to 'int f" "" { target *-*-* } .-1 }
> 
> base-commit: 084addf8a700fab9222d4127ab8524920d0ca481
> -- 
> 2.39.3
> 
> 

Re: [pushed] c++: fix tourney logic

[Patrick Palka]

On Fri, 27 Oct 2023, Patrick Palka wrote:

> On Fri, 20 Oct 2023, Jason Merrill wrote:
> 
> > Tested x86_64-pc-linux-gnu, applying to trunk.  Patrick, sorry I didn't apply
> > this sooner.
> > 
> > -- 8< --
> > 
> > In r13-3766 I changed the logic at the end of tourney to avoid redundant
> > comparisons, but the change also meant skipping any less-good matches
> > between the champ_compared_to_predecessor candidate and champ itself.
> > 
> > This should not be a correctness issue, since we believe that joust is a
> > partial order.  But it can lead to missed warnings, as in this testcase.
> 
> I suppose this rules out optimizing tourney via transitivity when in
> a non-SFINAE context since it'd cause missed warnings such as these.
> But maybe we'd still want to optimize the second pass via transitivity
> in a SFINAE context?

Eh, maybe it's not worth it either way..  According to some quick
experiments, making the second pass in tourney assume transitivity by
going up to the most recent tie even in non-SFINAE contexts reduces the
total number of non-trivial calls to tourney by about 5%.  Doing the
same in only SFINAE contexts reduces the number of calls by less than 1%.

> 
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* call.cc (tourney): Only skip champ_compared_to_predecessor.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/warn/Wsign-promo1.C: New test.
> > ---
> >  gcc/cp/call.cc                           |  5 +++--
> >  gcc/testsuite/g++.dg/warn/Wsign-promo1.C | 15 +++++++++++++++
> >  2 files changed, 18 insertions(+), 2 deletions(-)
> >  create mode 100644 gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> > 
> > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > index 657eca93d23..a49fde949d5 100644
> > --- a/gcc/cp/call.cc
> > +++ b/gcc/cp/call.cc
> > @@ -13227,10 +13227,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
> >       been compared to.  */
> >  
> >    for (challenger = candidates;
> > -       challenger != champ
> > -	 && challenger != champ_compared_to_predecessor;
> > +       challenger != champ;
> >         challenger = challenger->next)
> >      {
> > +      if (challenger == champ_compared_to_predecessor)
> > +	continue;
> >        fate = joust (champ, challenger, 0, complain);
> >        if (fate != 1)
> >  	return NULL;
> > diff --git a/gcc/testsuite/g++.dg/warn/Wsign-promo1.C b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> > new file mode 100644
> > index 00000000000..51b76eee735
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> > @@ -0,0 +1,15 @@
> > +// Check that we get joust warnings from comparing the final champ to a
> > +// candidate between it and the previous champ.
> > +
> > +// { dg-additional-options -Wsign-promo }
> > +
> > +struct A { A(int); };
> > +
> > +enum E { e };
> > +
> > +int f(int, A);
> > +int f(unsigned, A);
> > +int f(int, int);
> > +
> > +int i = f(e, 42);		// { dg-warning "passing 'E'" }
> > +// { dg-warning "in call to 'int f" "" { target *-*-* } .-1 }
> > 
> > base-commit: 084addf8a700fab9222d4127ab8524920d0ca481
> > -- 
> > 2.39.3
> > 
> > 
> 

Re: [pushed] c++: fix tourney logic

[Patrick Palka]

On Fri, 27 Oct 2023, Patrick Palka wrote:

> On Fri, 27 Oct 2023, Patrick Palka wrote:
> 
> > On Fri, 20 Oct 2023, Jason Merrill wrote:
> > 
> > > Tested x86_64-pc-linux-gnu, applying to trunk.  Patrick, sorry I didn't apply
> > > this sooner.
> > > 
> > > -- 8< --
> > > 
> > > In r13-3766 I changed the logic at the end of tourney to avoid redundant
> > > comparisons, but the change also meant skipping any less-good matches
> > > between the champ_compared_to_predecessor candidate and champ itself.
> > > 
> > > This should not be a correctness issue, since we believe that joust is a
> > > partial order.  But it can lead to missed warnings, as in this testcase.
> > 
> > I suppose this rules out optimizing tourney via transitivity when in
> > a non-SFINAE context since it'd cause missed warnings such as these.
> > But maybe we'd still want to optimize the second pass via transitivity
> > in a SFINAE context?
> 
> Eh, maybe it's not worth it either way..  According to some quick
> experiments, making the second pass in tourney assume transitivity by
> going up to the most recent tie even in non-SFINAE contexts reduces the
> total number of non-trivial calls to tourney by about 5%.  Doing the

total number of non-trivial calls to joust, rather

> same in only SFINAE contexts reduces the number of calls by less than 1%.
> 
> > 
> > > 
> > > gcc/cp/ChangeLog:
> > > 
> > > 	* call.cc (tourney): Only skip champ_compared_to_predecessor.
> > > 
> > > gcc/testsuite/ChangeLog:
> > > 
> > > 	* g++.dg/warn/Wsign-promo1.C: New test.
> > > ---
> > >  gcc/cp/call.cc                           |  5 +++--
> > >  gcc/testsuite/g++.dg/warn/Wsign-promo1.C | 15 +++++++++++++++
> > >  2 files changed, 18 insertions(+), 2 deletions(-)
> > >  create mode 100644 gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> > > 
> > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > > index 657eca93d23..a49fde949d5 100644
> > > --- a/gcc/cp/call.cc
> > > +++ b/gcc/cp/call.cc
> > > @@ -13227,10 +13227,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
> > >       been compared to.  */
> > >  
> > >    for (challenger = candidates;
> > > -       challenger != champ
> > > -	 && challenger != champ_compared_to_predecessor;
> > > +       challenger != champ;
> > >         challenger = challenger->next)
> > >      {
> > > +      if (challenger == champ_compared_to_predecessor)
> > > +	continue;
> > >        fate = joust (champ, challenger, 0, complain);
> > >        if (fate != 1)
> > >  	return NULL;
> > > diff --git a/gcc/testsuite/g++.dg/warn/Wsign-promo1.C b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> > > new file mode 100644
> > > index 00000000000..51b76eee735
> > > --- /dev/null
> > > +++ b/gcc/testsuite/g++.dg/warn/Wsign-promo1.C
> > > @@ -0,0 +1,15 @@
> > > +// Check that we get joust warnings from comparing the final champ to a
> > > +// candidate between it and the previous champ.
> > > +
> > > +// { dg-additional-options -Wsign-promo }
> > > +
> > > +struct A { A(int); };
> > > +
> > > +enum E { e };
> > > +
> > > +int f(int, A);
> > > +int f(unsigned, A);
> > > +int f(int, int);
> > > +
> > > +int i = f(e, 42);		// { dg-warning "passing 'E'" }
> > > +// { dg-warning "in call to 'int f" "" { target *-*-* } .-1 }
> > > 
> > > base-commit: 084addf8a700fab9222d4127ab8524920d0ca481
> > > -- 
> > > 2.39.3
> > > 
> > > 
> > 
>