[PATCH] Fix ix86_expand_int_movcc (PR target/64338)

[Uros Bizjak]

On Thu, Jan 8, 2015 at 6:09 PM, Jakub Jelinek <jakub@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  <jakub@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