[Bug middle-end/56552] New: conditional move can generate unnecessary conversion code

[Bug middle-end/56552] New: conditional move can generate unnecessary conversion code

[sje at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

             Bug #: 56552
           Summary: conditional move can generate unnecessary conversion
                    code
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: sje@gcc.gnu.org
            Target: mips*-*-*


With this test program:

unsigned short foo(unsigned short a1, unsigned short a2)
{
  unsigned short i, x;
  for (i = 0; i < 8; i++) {
    x = (a1 & 1) ^ (a2 & 1);
    a1 >>= 1;
    if (x == 1) a2 ^= 0x2006;
    a2 >>= 1;
    if (x == 1) a2 |= 0x8800;
    else        a2 &= 0x77ff;
  }
  return a2;
}

The GCC 4.8 MIPS compiler generates a 'andi r,r,0xffff' instruction after
a movz (conditional move) that was not present in GCC 4.7.  This instruction
is generated because MIPS does not implement a 'short' conditional move so
expand_cond_expr_using_cmove promotes the mode then it has to 'demote' it back
to short and that generates the andi instruction.  But if the source and
destination of the conditional move are both shorts to begin with this andi
instruction is not needed.

The problem was fixed with this patch:

http://gcc.gnu.org/ml/gcc-patches/2012-11/msg01148.html

However it caused bug PR56548 (ICE on x86) and the fix for that
bug causes the extra andi instruction to once again be generated.

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2013-03-07
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> 2013-03-07 10:05:50 UTC ---
Confirmed.  known-zero-bits and friends should figure this out I think.

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-03-07 13:32:30 UTC ---
I had to use -O3 -march=loongson2f to trigger it.

During combine, it seems the combiner figures everything out, but mips.md lacks
needed conditional move patterns:
(insn 36 35 37 2 (set (reg:SI 298)
        (if_then_else:SI (eq:SI (reg:SI 299)
                (const_int 0 [0]))
            (reg/v:SI 194 [ a2+-2 ])
            (reg/v:SI 213 [ a2+-2 ]))) 602 {*movsi_on_si}
     (expr_list:REG_DEAD (reg:SI 299)
        (expr_list:REG_DEAD (reg/v:SI 213 [ a2+-2 ])
            (expr_list:REG_DEAD (reg/v:SI 194 [ a2+-2 ])
                (nil)))))
(insn 37 36 40 2 (set (reg/v:SI 214 [ a2+-2 ])
        (zero_extend:SI (subreg:HI (reg:SI 298) 2))) 188 {*zero_extendhisi2}
     (expr_list:REG_DEAD (reg:SI 298)
        (nil)))

and combiner has:
Trying 36 -> 37:
Failed to match this instruction:
(set (reg/v:SI 214 [ a2+-2 ])
    (if_then_else:SI (reg:SI 299)
        (reg/v:SI 213 [ a2+-2 ])
        (reg/v:SI 194 [ a2+-2 ])))

I.e. it figures that the masking isn't needed, but during
simplification/canonicalization transforms that (ne:SI (reg:SI 299) (const_int
0)) in first argument of IF_THEN_ELSE into (reg:SI 299).
If mips.md had patterns that accepted for movcc the first argument of
IF_THEN_ELSE being a register_operand of GPR mode as if it was (ne:GPR (that
register) (const_int 0)), the testcase would be fixed.

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |pinskia at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> 2013-03-07 14:53:07 UTC ---
(In reply to comment #2)
> I had to use -O3 -march=loongson2f to trigger it.
> and combiner has:
> Trying 36 -> 37:
> Failed to match this instruction:
> (set (reg/v:SI 214 [ a2+-2 ])
>     (if_then_else:SI (reg:SI 299)
>         (reg/v:SI 213 [ a2+-2 ])
>         (reg/v:SI 194 [ a2+-2 ])))

Then this is mine.  I think Richard S. already approved this patch too.

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[rsandifo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rsandifo at gcc dot gnu.org

--- Comment #4 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
I've probably missed the bigger picture here, but FWIW, I don't
see any problem with extending *mov<GPR:mode>_on_<MOVECC:mode>
to QI and HI.  I suppose it would just be a case of replacing
GPR in that pattern with a new iterator.

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[rsandifo at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

--- Comment #5 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
(In reply to rsandifo@gcc.gnu.org from comment #4)
> I've probably missed the bigger picture here, but FWIW, I don't
> see any problem with extending *mov<GPR:mode>_on_<MOVECC:mode>
> to QI and HI.  I suppose it would just be a case of replacing
> GPR in that pattern with a new iterator.

Er, to be clear: I mean doing that on top of anything else that
might be needed.  Andrew's patch still looks good too.

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[sje at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

--- Comment #6 from Steve Ellcey <sje at gcc dot gnu.org> ---
Created attachment 31076
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31076&action=edit
Proposed patch I tested

Andrew, is this still on your TODO list?  I have attached a patch that I have
tested here.

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[sje at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

--- Comment #7 from Steve Ellcey <sje at gcc dot gnu.org> ---
Author: sje
Date: Mon Nov 18 19:20:12 2013
New Revision: 204979

URL: http://gcc.gnu.org/viewcvs?rev=204979&root=gcc&view=rev
Log:
2013-11-18  Andrew Pinski <apinski@cavium.com>
        Steve Ellcey  <sellcey@mips.com>

    PR target/56552
    * config/mips/mips.md (*mov<GPR:mode>_on_<MOVECC:mode>): Remove
    type restriction from equality_operator on conditonal move.
    (*mov<SCALARF:mode>_on_<MOVECC:mode>): Ditto.
    (*mov<GPR:mode>_on_<GPR2:mode>_ne): New.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/mips/mips.md

[Bug middle-end/56552] conditional move can generate unnecessary conversion code

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56552

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Fixed.