| Message ID | 001201d9b684$e319fdd0$a94df970$@nextmovesoftware.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix bootstrap failure (with g++ 4.8.5) in tree-if-conv.cc. | expand |
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 > > -- > > >
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 > > -- > > >
> -----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
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
> 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
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
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
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; });
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 --