[Bug tree-optimization/63743] New: Thumb1: big regression for float operators by r216728

[Bug tree-optimization/63743] New: Thumb1: big regression for float operators by r216728

[zhenqiang.chen at arm dot com]

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

            Bug ID: 63743
           Summary: Thumb1: big regression for float operators by r216728
           Product: gcc
           Version: 5.0
            Status: UNCONFIRMED
          Severity: critical
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zhenqiang.chen at arm dot com

Created attachment 33887
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=33887&action=edit
test case

Root cause: the fold_stmt swaps the operands, which leads to register shuffle.

commit f619ecaed41d1487091098a0f4fdf4d6ed1fa379
Author: rguenth <rguenth@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Mon Oct 27 11:30:23 2014 +0000

    2014-10-27  Richard Biener  <rguenther@suse.de>

        * tree-ssa-forwprop.c: Include tree-cfgcleanup.h and tree-into-ssa.h.
        (lattice): New global.
        (fwprop_ssa_val): New function.
        (fold_all_stmts): Likewise.
        (pass_forwprop::execute): Finally fold all stmts.

        * gcc.dg/tree-ssa/forwprop-6.c: Scan ccp1 dump instead.
        * gcc.dg/strlenopt-8.c: Adjust and XFAIL for non_strict_align
        target due to memcpy inline-expansion.


    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@216728
138bc75d-0d04-0410-961f-82ee72b054a4

A simplified case is attached.

Options: -mthumb -Os -mcpu=cortex-m0

Before the patch, tree codes like

_20 = _14 + _19;
_21 = _20 * x_13;

After the patch, tree codes like

_20 = _14 + _19;
_21 = x_13 * _20;

Without HARD fpu support, all operators will be changed to function calls. The
assemble codes change like:

Before the patch,
        bl      __aeabi_dadd
        ldr     r2, [sp]
        ldr     r3, [sp, #4]
        /* r0, r1 are reused from the return values of the previous call. */
        bl      __aeabi_dmul

After the patch,
        bl      __aeabi_dadd
        mov     r2, r0
        mov     r3, r1
        ldr     r0, [sp]
        ldr     r1, [sp, #4]
        bl      __aeabi_dmul

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Were we swapping operands before?  I mean, if you rewrite the testcase to swap
the * arguments in the source, did you get the same more efficient code in the
past?

In any case, this doesn't sound something that we should keep in mind in GIMPLE
passes, + and * are just commutative, how they are expanded is a matter of
expansion.  So, either look for this during expansion (if a commutative
operation is being expanded using libcall (or is this emitted by the backend?),
see if one of the operands isn't result of immediately preceeding emitted call
and if it is, perhaps swap the order of arguments), or add some register
allocator smarts (add a way to mark library calls as commutative and allow RA
to swap arguments to them if beneficial).

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[zhenqiang.chen at arm dot com]

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

--- Comment #2 from Zhenqiang Chen <zhenqiang.chen at arm dot com> ---
(In reply to Jakub Jelinek from comment #1)
> Were we swapping operands before?  I mean, if you rewrite the testcase to
> swap the * arguments in the source, did you get the same more efficient code
> in the past?

Yes. I tried the test case:

double
test1 (double x, double y)
{
  return x * (x + y);
}
double
test2 (double x, double y)
{
  return (x + y) * x;
}

Without r216728, I got efficient codes for both functions. But with r216728, I
got inefficient codes for both functions.

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Zhenqiang Chen from comment #2)
> (In reply to Jakub Jelinek from comment #1)
> > Were we swapping operands before?  I mean, if you rewrite the testcase to
> > swap the * arguments in the source, did you get the same more efficient code
> > in the past?
> 
> Yes. I tried the test case:
> 
> double
> test1 (double x, double y)
> {
>   return x * (x + y);
> }
> double
> test2 (double x, double y)
> {
>   return (x + y) * x;
> }
> 
> Without r216728, I got efficient codes for both functions. But with r216728,
> I got inefficient codes for both functions.

What about
double
test3 (double x, double y)
{
  return (x + y) * (x - y);
}
?  At least from quick looking at ppc -msoft-float -O2 -m32, I see the same
issue there, add called first, sub called second, and result of second returned
in the same registers as used for the first argument. So something to handle at
expansion or RA rather than in GIMPLE anyway IMHO.

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[zhenqiang.chen at arm dot com]

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

--- Comment #4 from Zhenqiang Chen <zhenqiang.chen at arm dot com> ---
(In reply to Jakub Jelinek from comment #3)
> (In reply to Zhenqiang Chen from comment #2)
> > (In reply to Jakub Jelinek from comment #1)
> > > Were we swapping operands before?  I mean, if you rewrite the testcase to
> > > swap the * arguments in the source, did you get the same more efficient code
> > > in the past?
> > 
> > Yes. I tried the test case:
> > 
> > double
> > test1 (double x, double y)
> > {
> >   return x * (x + y);
> > }
> > double
> > test2 (double x, double y)
> > {
> >   return (x + y) * x;
> > }
> > 
> > Without r216728, I got efficient codes for both functions. But with r216728,
> > I got inefficient codes for both functions.
> 
> What about
> double
> test3 (double x, double y)
> {
>   return (x + y) * (x - y);
> }
> ?  At least from quick looking at ppc -msoft-float -O2 -m32, I see the same
> issue there, add called first, sub called second, and result of second
> returned in the same registers as used for the first argument. So something
> to handle at expansion or RA rather than in GIMPLE anyway IMHO.

Same issue for the case on Thumb1.

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[rguenth at gcc dot gnu.org]

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

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

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

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
This is just operand canonicalization according to tree_swap_operands which is
now consistently applied.  It was a bug that it wasn't applied before (the
usual offender here is SSA rewriting that doesn't fold "changed" statements
nor re-canonicalizes).

You are just (un)lucky btw, try both

double
test1 (double x, double y)
{
  double tem = x + y;
  double tem2 = x;
  return tem2 * tem;
}

and

double
test1 (double x, double y)
{
  double tem = x + y;
  double tem2 = x;
  return tem * tem2;
}

there is nothing that forces ordering in the way you would prefer it.  So as
Jakub says - this needs addressing in RTL expansion and/or TER and SSA
coalescing.

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[ysrumyan at gmail dot com]

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

Yuri Rumyantsev <ysrumyan at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ysrumyan at gmail dot com

--- Comment #6 from Yuri Rumyantsev <ysrumyan at gmail dot com> ---
We also noticed regression on x86 32-bit targets after this fix and prepared a
simple fix which cures it: swap operands of commutative operations is performed
if cost of second operand is higher than the first one (the operand cost is
number of statements required for producing it). It is done at expand phase
inside expand_gimple_basic_block.

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2015-01-15
     Ever confirmed|0                           |1

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Was it fixed now?

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[thopre01 at gcc dot gnu.org]

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

Thomas Preud'homme <thopre01 at gcc dot gnu.org> changed:

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

--- Comment #8 from Thomas Preud'homme <thopre01 at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #7)
> Was it fixed now?

I can still reproduce it. I'll try to look into it next month (and will assign
it to myself when I do).

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[thopre01 at gcc dot gnu.org]

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

Thomas Preud'homme <thopre01 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |thopre01 at gcc dot gnu.org

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[thopre01 at gcc dot gnu.org]

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

--- Comment #9 from Thomas Preud'homme <thopre01 at gcc dot gnu.org> ---
Author: thopre01
Date: Mon Mar  9 01:31:42 2015
New Revision: 221276

URL: https://gcc.gnu.org/viewcvs?rev=221276&root=gcc&view=rev
Log:
2015-03-09  Thomas Preud'homme  <thomas.preudhomme@arm.com>

    gcc/
    PR tree-optimization/63743
    * cfgexpand.c (reorder_operands): Also reorder if only second operand
    had its definition forwarded by TER.

    gcc/testsuite/
    PR tree-optimization/63743
    * gcc.dg/pr63743.c: New test.

Added:
    trunk/gcc/testsuite/gcc.dg/pr63743.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cfgexpand.c
    trunk/gcc/testsuite/ChangeLog

[Bug tree-optimization/63743] Thumb1: big regression for float operators by r216728

[thopre01 at gcc dot gnu.org]

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

Thomas Preud'homme <thopre01 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
      Known to work|                            |5.0
         Resolution|---                         |FIXED

--- Comment #10 from Thomas Preud'homme <thopre01 at gcc dot gnu.org> ---
Regression solved on trunk.