[PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Roger Sayle]

[-- Attachment #1.1: Type: text/plain, Size: 381 bytes --]

 

This patch fixes the bootstrap failure I'm seeing using gcc 4.8.5 as

the host compiler.  Ok for mainline?  [I might be missing something]

 

 

2023-07-14  Roger Sayle  <roger@nextmovesoftware.com>

 

gcc/ChangeLog

        * tree-if-conv.cc (predicate_scalar_phi): Make the arguments

        to the std::sort comparison lambda function const.

 

 

Cheers,

Roger

--

 


[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 524 bytes --]

diff --git a/gcc/tree-if-conv.cc b/gcc/tree-if-conv.cc
index 91e2eff..799f071 100644
--- a/gcc/tree-if-conv.cc
+++ b/gcc/tree-if-conv.cc
@@ -2204,7 +2204,8 @@ predicate_scalar_phi (gphi *phi, gimple_stmt_iterator *gsi)
     }
 
   /* Sort elements based on rankings ARGS.  */
-  std::sort(argsKV.begin(), argsKV.end(), [](ArgEntry &left, ArgEntry &right) {
+  std::sort(argsKV.begin(), argsKV.end(), [](const ArgEntry &left,
+					     const ArgEntry &right) {
     return left.second < right.second;
   });
 

Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Andrew Pinski]

On Fri, Jul 14, 2023 at 11:56 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
>
> This patch fixes the bootstrap failure I'm seeing using gcc 4.8.5 as
>
> the host compiler.  Ok for mainline?  [I might be missing something]

I think adding const here makes this well defined C++20 too.
See http://cplusplus.github.io/LWG/lwg-defects.html#3031 .
Also see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107850 .

(I could be reading these wrong too).

Thanks,
Andrew

>
>
>
>
>
> 2023-07-14  Roger Sayle  <roger@nextmovesoftware.com>
>
>
>
> gcc/ChangeLog
>
>         * tree-if-conv.cc (predicate_scalar_phi): Make the arguments
>
>         to the std::sort comparison lambda function const.
>
>
>
>
>
> Cheers,
>
> Roger
>
> --
>
>
>

Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Richard Biener]

On Fri, Jul 14, 2023 at 8:56 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
>
> This patch fixes the bootstrap failure I'm seeing using gcc 4.8.5 as
>
> the host compiler.  Ok for mainline?  [I might be missing something]

OK.   Btw, while I didn't spot this during review I would appreciate
if the code could use vec.[q]sort, this should work with a lambda as
well I think.

>
>
>
>
> 2023-07-14  Roger Sayle  <roger@nextmovesoftware.com>
>
>
>
> gcc/ChangeLog
>
>         * tree-if-conv.cc (predicate_scalar_phi): Make the arguments
>
>         to the std::sort comparison lambda function const.
>
>
>
>
>
> Cheers,
>
> Roger
>
> --
>
>
>

RE: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Tamar Christina]

> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Monday, July 17, 2023 7:19 AM
> To: Roger Sayle <roger@nextmovesoftware.com>
> Cc: gcc-patches@gcc.gnu.org; Tamar Christina <Tamar.Christina@arm.com>
> Subject: Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.
> 
> On Fri, Jul 14, 2023 at 8:56 PM Roger Sayle <roger@nextmovesoftware.com>
> wrote:
> >
> >
> >
> > This patch fixes the bootstrap failure I'm seeing using gcc 4.8.5 as
> >
> > the host compiler.  Ok for mainline?  [I might be missing something]
> 
> OK.   Btw, while I didn't spot this during review I would appreciate
> if the code could use vec.[q]sort, this should work with a lambda as well I
> think.

That was my first use, but that hits https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99469

Regards,
Tamar

Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Andrew Pinski]

On Mon, Jul 17, 2023 at 12:21 AM Tamar Christina via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> > -----Original Message-----
> > From: Richard Biener <richard.guenther@gmail.com>
> > Sent: Monday, July 17, 2023 7:19 AM
> > To: Roger Sayle <roger@nextmovesoftware.com>
> > Cc: gcc-patches@gcc.gnu.org; Tamar Christina <Tamar.Christina@arm.com>
> > Subject: Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.
> >
> > On Fri, Jul 14, 2023 at 8:56 PM Roger Sayle <roger@nextmovesoftware.com>
> > wrote:
> > >
> > >
> > >
> > > This patch fixes the bootstrap failure I'm seeing using gcc 4.8.5 as
> > >
> > > the host compiler.  Ok for mainline?  [I might be missing something]
> >
> > OK.   Btw, while I didn't spot this during review I would appreciate
> > if the code could use vec.[q]sort, this should work with a lambda as well I
> > think.
>
> That was my first use, but that hits https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99469

That is not hitting PR 99469 but rather it means your comparison is
not correct for an (unstable) sort.
That is qsort comparator should have this relationship `f(a,b) ==
!f(b, a)` and `f(a,a)` should also return false.
If you are running into this for qsort here, you will most likely run
into issues with std::sort later on too.

Thanks,
Andrew

>
> Regards,
> Tamar

RE: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Tamar Christina]

> On Mon, Jul 17, 2023 at 12:21 AM Tamar Christina via Gcc-patches <gcc-
> patches@gcc.gnu.org> wrote:
> >
> > > -----Original Message-----
> > > From: Richard Biener <richard.guenther@gmail.com>
> > > Sent: Monday, July 17, 2023 7:19 AM
> > > To: Roger Sayle <roger@nextmovesoftware.com>
> > > Cc: gcc-patches@gcc.gnu.org; Tamar Christina
> > > <Tamar.Christina@arm.com>
> > > Subject: Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-
> conv.cc.
> > >
> > > On Fri, Jul 14, 2023 at 8:56 PM Roger Sayle
> > > <roger@nextmovesoftware.com>
> > > wrote:
> > > >
> > > >
> > > >
> > > > This patch fixes the bootstrap failure I'm seeing using gcc 4.8.5
> > > > as
> > > >
> > > > the host compiler.  Ok for mainline?  [I might be missing
> > > > something]
> > >
> > > OK.   Btw, while I didn't spot this during review I would appreciate
> > > if the code could use vec.[q]sort, this should work with a lambda as
> > > well I think.
> >
> > That was my first use, but that hits
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99469
> 
> That is not hitting PR 99469 but rather it means your comparison is not
> correct for an (unstable) sort.
> That is qsort comparator should have this relationship `f(a,b) == !f(b, a)` and
> `f(a,a)` should also return false.
	
I'm using the standard std::pair comparator which indicates that f(a,a) is true,
https://en.cppreference.com/w/cpp/utility/pair/operator_cmp 

> If you are running into this for qsort here, you will most likely run into issues
> with std::sort later on too.

Don't see why or how. It needs to have a consistent relationship which std::pair
maintains.  So why would using the standard tuple comparator with a standard
std::sort cause problem?

Thanks,
Tamar

> 
> Thanks,
> Andrew
> 
> >
> > Regards,
> > Tamar

Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Richard Biener]

On Mon, Jul 17, 2023 at 9:35 AM Tamar Christina <Tamar.Christina@arm.com> wrote:
>
> > On Mon, Jul 17, 2023 at 12:21 AM Tamar Christina via Gcc-patches <gcc-
> > patches@gcc.gnu.org> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Richard Biener <richard.guenther@gmail.com>
> > > > Sent: Monday, July 17, 2023 7:19 AM
> > > > To: Roger Sayle <roger@nextmovesoftware.com>
> > > > Cc: gcc-patches@gcc.gnu.org; Tamar Christina
> > > > <Tamar.Christina@arm.com>
> > > > Subject: Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-
> > conv.cc.
> > > >
> > > > On Fri, Jul 14, 2023 at 8:56 PM Roger Sayle
> > > > <roger@nextmovesoftware.com>
> > > > wrote:
> > > > >
> > > > >
> > > > >
> > > > > This patch fixes the bootstrap failure I'm seeing using gcc 4.8.5
> > > > > as
> > > > >
> > > > > the host compiler.  Ok for mainline?  [I might be missing
> > > > > something]
> > > >
> > > > OK.   Btw, while I didn't spot this during review I would appreciate
> > > > if the code could use vec.[q]sort, this should work with a lambda as
> > > > well I think.
> > >
> > > That was my first use, but that hits
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99469
> >
> > That is not hitting PR 99469 but rather it means your comparison is not
> > correct for an (unstable) sort.
> > That is qsort comparator should have this relationship `f(a,b) == !f(b, a)` and
> > `f(a,a)` should also return false.
>
> I'm using the standard std::pair comparator which indicates that f(a,a) is true,
> https://en.cppreference.com/w/cpp/utility/pair/operator_cmp
>
> > If you are running into this for qsort here, you will most likely run into issues
> > with std::sort later on too.
>
> Don't see why or how. It needs to have a consistent relationship which std::pair
> maintains.  So why would using the standard tuple comparator with a standard
> std::sort cause problem?

At least for

     return left.second < right.second;

f(a,a) doesn't hold.  Note qsort can end up comparing an element to
itself (not sure
if GCCs implementation now can).

Richard.

> Thanks,
> Tamar
>
> >
> > Thanks,
> > Andrew
> >
> > >
> > > Regards,
> > > Tamar

Re: [PATCH] Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc.

[Alexander Monakov]

On Mon, 17 Jul 2023, Richard Biener wrote:

> > > > > OK.   Btw, while I didn't spot this during review I would appreciate
> > > > > if the code could use vec.[q]sort, this should work with a lambda as
> > > > > well I think.
> > > >
> > > > That was my first use, but that hits
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99469
> > >
> > > That is not hitting PR 99469 but rather it means your comparison is not
> > > correct for an (unstable) sort.
> > > That is qsort comparator should have this relationship `f(a,b) == !f(b, a)` and
> > > `f(a,a)` should also return false.
> >
> > I'm using the standard std::pair comparator which indicates that f(a,a) is true,
> > https://en.cppreference.com/w/cpp/utility/pair/operator_cmp
> >
> > > If you are running into this for qsort here, you will most likely run into issues
> > > with std::sort later on too.
> >
> > Don't see why or how. It needs to have a consistent relationship which std::pair
> > maintains.  So why would using the standard tuple comparator with a standard
> > std::sort cause problem?
> 
> At least for
> 
>      return left.second < right.second;
> 
> f(a,a) doesn't hold.  Note qsort can end up comparing an element to
> itself (not sure if GCCs implementation now can).

(it cannot but that is not important here)

Tamar, while std::sort receives a "less-than" comparison predicate, qsort
needs a tri-state comparator that returns a negative value for "less-than"
relation, positive for "more-than", and zero when operands are "equal".

Passing output of std::pair::operator< straight to qsort is not correct,
and qsort_chk catches that mistake at runtime.

std::sort is not a stable sort and therefore can cause code generation
differences by swapping around elements that are not bitwise-identical
but "equal" according to the comparator. This is the main reason for
preferring our internal qsort, which yields same results on all platforms.

Let me also note that #include <algorithm> is pretty heavy-weight, and so
I'd suggest to avoid it to avoid needlessly increasing bootstrap times.

Alexander