Re: [PATCH] Fix ix86_expand_int_movcc (PR target/64338)
On Thu, Jan 8, 2015 at 6:09 PM, Jakub Jelinek ja...@redhat.com wrote:
Hi!
The recent ifcvt changes result in movcc being attempted with
comparisons like (ltgt (reg:CCFPU flags) (const_int 0)).
I see several issues with the current ix86_expand_int_movcc code:
1) the code was unprepared to handle *reverse_condition* failures
(returns of UNKNOWN)
2) for CCFP/CCFPU modes, I think it should be treated like scalar
float comparisons, ix86_reverse_condition seems to do the job here
3) compare_code in the second hunk was a dead computation, because
the variable is not used afterwards until it is unconditionally overwritten
(set to UNKNOWN).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2015-01-08 Jakub Jelinek ja...@redhat.com
PR target/64338
* config/i386/i386.c (ix86_expand_int_movcc): Don't reverse
compare_code when it is unconditionally overwritten afterwards.
Use ix86_reverse_condition instead of reverse_condition. Don't
change code if *reverse_condition* returned UNKNOWN and don't
swap ct/cf and negate diff in that case.
* g++.dg/opt/pr64338.C: New test.
OK with two small nits inline.
Thanks,
Uros.
--- gcc/config/i386/i386.c.jj 2015-01-06 09:14:05.0 +0100
+++ gcc/config/i386/i386.c 2015-01-07 09:59:09.297790590 +0100
@@ -20830,9 +20830,7 @@ ix86_expand_int_movcc (rtx operands[])
if (diff 0)
{
machine_mode cmp_mode = GET_MODE (op0);
-
- std::swap (ct, cf);
- diff = -diff;
+ enum rtx_code new_code;
if (SCALAR_FLOAT_MODE_P (cmp_mode))
{
@@ -20842,13 +20840,15 @@ ix86_expand_int_movcc (rtx operands[])
is not valid in general (we may convert non-trapping
condition
to trapping one), however on i386 we currently emit all
comparisons unordered. */
- compare_code = reverse_condition_maybe_unordered (compare_code);
- code = reverse_condition_maybe_unordered (code);
+ new_code = reverse_condition_maybe_unordered (code);
}
else
+ new_code = ix86_reverse_condition (code, cmp_mode);
+ if (new_code != UNKNOWN)
{
- compare_code = reverse_condition (compare_code);
- code = reverse_condition (code);
+ code = new_code;
+ std::swap (ct, cf);
+ diff = -diff;
Please put std::swap at the top, above code= assignment. Cosmetic, but
I noticed this during std::swap conversion. ;)
}
}
@@ -20986,9 +20986,7 @@ ix86_expand_int_movcc (rtx operands[])
if (cf == 0)
{
machine_mode cmp_mode = GET_MODE (op0);
-
- cf = ct;
- ct = 0;
+ enum rtx_code new_code;
if (SCALAR_FLOAT_MODE_P (cmp_mode))
{
@@ -20998,14 +20996,21 @@ ix86_expand_int_movcc (rtx operands[])
that is not valid in general (we may convert non-trapping
condition to trapping one), however on i386 we currently
emit all comparisons unordered. */
- code = reverse_condition_maybe_unordered (code);
+ new_code = reverse_condition_maybe_unordered (code);
}
else
{
- code = reverse_condition (code);
- if (compare_code != UNKNOWN)
+ new_code = ix86_reverse_condition (code, cmp_mode);
+ if (compare_code != UNKNOWN new_code != UNKNOWN)
compare_code = reverse_condition (compare_code);
}
+
+ if (new_code != UNKNOWN)
+ {
+ code = new_code;
+ cf = ct;
+ ct = 0;
+ }
}
if (compare_code != UNKNOWN)
--- gcc/testsuite/g++.dg/opt/pr64338.C.jj 2015-01-07 10:18:04.740275018
+0100
+++ gcc/testsuite/g++.dg/opt/pr64338.C 2015-01-07 10:17:50.0 +0100
@@ -0,0 +1,29 @@
+// PR target/64338
+// { dg-do compile }
+// { dg-options -O2 }
+// { dg-additional-options -mtune=generic -march=i586 { target { {
i?86-*-* x86_64-*-* } ia32 } } }
Please use -mtune=i686, generic tuning setting changes over time ...
+enum O {};
+struct A { A (); };
+struct B { int fn1 (); };
+struct C { struct D; D *fn2 (); void fn3 (); int fn4 (); };
+struct F { void fn5 (const int = 0); };
+struct G { F *fn6 (); };
+struct H { int h; };
+struct C::D { friend class C; G *fn7 (); };
+O a;
+
+void
+C::fn3 ()
+{
+ int b = a;
+ H c;
+ if (b)
+fn2 ()-fn7 ()-fn6 ()-fn5 ();
+ double d;
+ if (fn4 ())
+d = c.h 0;
+ A e (b ? A () : A ());
+ B f;
+ f.fn1 () d fn2 ();
+}
Jakub
[PATCH] Fix ix86_expand_int_movcc (PR target/64338)
Hi!
The recent ifcvt changes result in movcc being attempted with
comparisons like (ltgt (reg:CCFPU flags) (const_int 0)).
I see several issues with the current ix86_expand_int_movcc code:
1) the code was unprepared to handle *reverse_condition* failures
(returns of UNKNOWN)
2) for CCFP/CCFPU modes, I think it should be treated like scalar
float comparisons, ix86_reverse_condition seems to do the job here
3) compare_code in the second hunk was a dead computation, because
the variable is not used afterwards until it is unconditionally overwritten
(set to UNKNOWN).
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2015-01-08 Jakub Jelinek ja...@redhat.com
PR target/64338
* config/i386/i386.c (ix86_expand_int_movcc): Don't reverse
compare_code when it is unconditionally overwritten afterwards.
Use ix86_reverse_condition instead of reverse_condition. Don't
change code if *reverse_condition* returned UNKNOWN and don't
swap ct/cf and negate diff in that case.
* g++.dg/opt/pr64338.C: New test.
--- gcc/config/i386/i386.c.jj 2015-01-06 09:14:05.0 +0100
+++ gcc/config/i386/i386.c 2015-01-07 09:59:09.297790590 +0100
@@ -20830,9 +20830,7 @@ ix86_expand_int_movcc (rtx operands[])
if (diff 0)
{
machine_mode cmp_mode = GET_MODE (op0);
-
- std::swap (ct, cf);
- diff = -diff;
+ enum rtx_code new_code;
if (SCALAR_FLOAT_MODE_P (cmp_mode))
{
@@ -20842,13 +20840,15 @@ ix86_expand_int_movcc (rtx operands[])
is not valid in general (we may convert non-trapping condition
to trapping one), however on i386 we currently emit all
comparisons unordered. */
- compare_code = reverse_condition_maybe_unordered (compare_code);
- code = reverse_condition_maybe_unordered (code);
+ new_code = reverse_condition_maybe_unordered (code);
}
else
+ new_code = ix86_reverse_condition (code, cmp_mode);
+ if (new_code != UNKNOWN)
{
- compare_code = reverse_condition (compare_code);
- code = reverse_condition (code);
+ code = new_code;
+ std::swap (ct, cf);
+ diff = -diff;
}
}
@@ -20986,9 +20986,7 @@ ix86_expand_int_movcc (rtx operands[])
if (cf == 0)
{
machine_mode cmp_mode = GET_MODE (op0);
-
- cf = ct;
- ct = 0;
+ enum rtx_code new_code;
if (SCALAR_FLOAT_MODE_P (cmp_mode))
{
@@ -20998,14 +20996,21 @@ ix86_expand_int_movcc (rtx operands[])
that is not valid in general (we may convert non-trapping
condition to trapping one), however on i386 we currently
emit all comparisons unordered. */
- code = reverse_condition_maybe_unordered (code);
+ new_code = reverse_condition_maybe_unordered (code);
}
else
{
- code = reverse_condition (code);
- if (compare_code != UNKNOWN)
+ new_code = ix86_reverse_condition (code, cmp_mode);
+ if (compare_code != UNKNOWN new_code != UNKNOWN)
compare_code = reverse_condition (compare_code);
}
+
+ if (new_code != UNKNOWN)
+ {
+ code = new_code;
+ cf = ct;
+ ct = 0;
+ }
}
if (compare_code != UNKNOWN)
--- gcc/testsuite/g++.dg/opt/pr64338.C.jj 2015-01-07 10:18:04.740275018
+0100
+++ gcc/testsuite/g++.dg/opt/pr64338.C 2015-01-07 10:17:50.0 +0100
@@ -0,0 +1,29 @@
+// PR target/64338
+// { dg-do compile }
+// { dg-options -O2 }
+// { dg-additional-options -mtune=generic -march=i586 { target { { i?86-*-*
x86_64-*-* } ia32 } } }
+
+enum O {};
+struct A { A (); };
+struct B { int fn1 (); };
+struct C { struct D; D *fn2 (); void fn3 (); int fn4 (); };
+struct F { void fn5 (const int = 0); };
+struct G { F *fn6 (); };
+struct H { int h; };
+struct C::D { friend class C; G *fn7 (); };
+O a;
+
+void
+C::fn3 ()
+{
+ int b = a;
+ H c;
+ if (b)
+fn2 ()-fn7 ()-fn6 ()-fn5 ();
+ double d;
+ if (fn4 ())
+d = c.h 0;
+ A e (b ? A () : A ());
+ B f;
+ f.fn1 () d fn2 ();
+}
Jakub
Re: [PATCH] Fix ix86_expand_int_movcc (PR target/64338)
On Thu, Jan 08, 2015 at 07:37:44PM +0100, Uros Bizjak wrote:
Please put std::swap at the top, above code= assignment. Cosmetic, but
I noticed this during std::swap conversion. ;)
Ok. Put there also the diff = -diff; assignment and cf = ct; ct = 0; swap
to keep the order that used to be there before.
--- gcc/testsuite/g++.dg/opt/pr64338.C.jj 2015-01-07
10:18:04.740275018 +0100
+++ gcc/testsuite/g++.dg/opt/pr64338.C 2015-01-07 10:17:50.0 +0100
@@ -0,0 +1,29 @@
+// PR target/64338
+// { dg-do compile }
+// { dg-options -O2 }
+// { dg-additional-options -mtune=generic -march=i586 { target { {
i?86-*-* x86_64-*-* } ia32 } } }
Please use -mtune=i686, generic tuning setting changes over time ...
That doesn't ICE without the patch, so I've changed it to -mtune=nehalem
instead. So, here is what I've checked in.
2015-01-08 Jakub Jelinek ja...@redhat.com
PR target/64338
* config/i386/i386.c (ix86_expand_int_movcc): Don't reverse
compare_code when it is unconditionally overwritten afterwards.
Use ix86_reverse_condition instead of reverse_condition. Don't
change code if *reverse_condition* returned UNKNOWN and don't
swap ct/cf and negate diff in that case.
* g++.dg/opt/pr64338.C: New test.
--- gcc/config/i386/i386.c.jj 2015-01-08 10:19:00.337260146 +0100
+++ gcc/config/i386/i386.c 2015-01-08 20:09:30.716001199 +0100
@@ -20830,9 +20830,7 @@ ix86_expand_int_movcc (rtx operands[])
if (diff 0)
{
machine_mode cmp_mode = GET_MODE (op0);
-
- std::swap (ct, cf);
- diff = -diff;
+ enum rtx_code new_code;
if (SCALAR_FLOAT_MODE_P (cmp_mode))
{
@@ -20842,13 +20840,15 @@ ix86_expand_int_movcc (rtx operands[])
is not valid in general (we may convert non-trapping condition
to trapping one), however on i386 we currently emit all
comparisons unordered. */
- compare_code = reverse_condition_maybe_unordered (compare_code);
- code = reverse_condition_maybe_unordered (code);
+ new_code = reverse_condition_maybe_unordered (code);
}
else
+ new_code = ix86_reverse_condition (code, cmp_mode);
+ if (new_code != UNKNOWN)
{
- compare_code = reverse_condition (compare_code);
- code = reverse_condition (code);
+ std::swap (ct, cf);
+ diff = -diff;
+ code = new_code;
}
}
@@ -20986,9 +20986,7 @@ ix86_expand_int_movcc (rtx operands[])
if (cf == 0)
{
machine_mode cmp_mode = GET_MODE (op0);
-
- cf = ct;
- ct = 0;
+ enum rtx_code new_code;
if (SCALAR_FLOAT_MODE_P (cmp_mode))
{
@@ -20998,14 +20996,21 @@ ix86_expand_int_movcc (rtx operands[])
that is not valid in general (we may convert non-trapping
condition to trapping one), however on i386 we currently
emit all comparisons unordered. */
- code = reverse_condition_maybe_unordered (code);
+ new_code = reverse_condition_maybe_unordered (code);
}
else
{
- code = reverse_condition (code);
- if (compare_code != UNKNOWN)
+ new_code = ix86_reverse_condition (code, cmp_mode);
+ if (compare_code != UNKNOWN new_code != UNKNOWN)
compare_code = reverse_condition (compare_code);
}
+
+ if (new_code != UNKNOWN)
+ {
+ cf = ct;
+ ct = 0;
+ code = new_code;
+ }
}
if (compare_code != UNKNOWN)
--- gcc/testsuite/g++.dg/opt/pr64338.C.jj 2015-01-08 20:07:09.790383489
+0100
+++ gcc/testsuite/g++.dg/opt/pr64338.C 2015-01-08 20:11:26.756039590 +0100
@@ -0,0 +1,29 @@
+// PR target/64338
+// { dg-do compile }
+// { dg-options -O2 }
+// { dg-additional-options -mtune=nehalem -march=i586 { target { { i?86-*-*
x86_64-*-* } ia32 } } }
+
+enum O {};
+struct A { A (); };
+struct B { int fn1 (); };
+struct C { struct D; D *fn2 (); void fn3 (); int fn4 (); };
+struct F { void fn5 (const int = 0); };
+struct G { F *fn6 (); };
+struct H { int h; };
+struct C::D { friend class C; G *fn7 (); };
+O a;
+
+void
+C::fn3 ()
+{
+ int b = a;
+ H c;
+ if (b)
+fn2 ()-fn7 ()-fn6 ()-fn5 ();
+ double d;
+ if (fn4 ())
+d = c.h 0;
+ A e (b ? A () : A ());
+ B f;
+ f.fn1 () d fn2 ();
+}
Jakub
