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.000000000 +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.000000000 +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
