[Bug c++/66119] New: Regression in optimization of avx-code

[Bug c++/66119] New: Regression in optimization of avx-code

[joachim.schoeberl at tuwien dot ac.at]

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

            Bug ID: 66119
           Summary: Regression in optimization of avx-code
           Product: gcc
           Version: 5.1.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: joachim.schoeberl at tuwien dot ac.at
  Target Milestone: ---

Created attachment 35525
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35525&action=edit
testcode

gcc 5.1 produces a lot of scalar moves for the attached vector-class code. 
gcc 4.9 generates compact code (see below).

compiled using:
gcc -O3 -mavx -S -std=c++11 testgcc.cpp

compiler version:
gcc (GCC) 5.1.1 20150505



gcc 5.1 works fine in any of the cases:

- we use a manual copy constructor instead of '= default' (line 37):
MyTSIMD (const MyTSIMD & s2) : data(s2.data) { ; } 

- we use the concrete vector-class instead of the template (line 45):
using MyVec = MyAVX;

- we do not use  
  __attribute__ ((__always_inline__)) 
for ComputeSomething  (line 58)



Cheers, Joachim




code generated by gcc5.1:

        .globl  _Z12TestFunction4Vec2S_
        .type   _Z12TestFunction4Vec2S_, @function
_Z12TestFunction4Vec2S_:
.LFB4604:
        .cfi_startproc
        movq    72(%rsp), %rdx
        vmovapd 40(%rsp), %ymm0
        movq    %rdi, %rax
        vmovapd 8(%rsp), %ymm1
        movq    %rdx, -88(%rsp)
        movq    80(%rsp), %rdx
        movq    %rdx, -80(%rsp)
        movq    88(%rsp), %rdx
        movq    %rdx, -72(%rsp)
        movq    96(%rsp), %rdx
        movq    %rdx, -64(%rsp)
        movq    104(%rsp), %rdx
        vaddpd  -88(%rsp), %ymm1, %ymm1
        movq    %rdx, -56(%rsp)
        movq    112(%rsp), %rdx
        movq    %rdx, -48(%rsp)
        movq    120(%rsp), %rdx
        vmovapd %ymm1, (%rdi)
        movq    %rdx, -40(%rsp)
        movq    128(%rsp), %rdx
        movq    %rdx, -32(%rsp)
        vaddpd  -56(%rsp), %ymm0, %ymm0
        vmovapd %ymm0, 32(%rdi)
        vzeroupper
        ret
        .cfi_endproc




code generated by gcc 4.9.2:

        .type   _Z12TestFunction4Vec2S_, @function
_Z12TestFunction4Vec2S_:
.LFB2234:
        .cfi_startproc
        vmovapd 40(%rsp), %ymm0
        movq    %rdi, %rax
        vmovapd 8(%rsp), %ymm1
        vaddpd  104(%rsp), %ymm0, %ymm0
        vaddpd  72(%rsp), %ymm1, %ymm1
        vmovapd %ymm0, 32(%rdi)
        vmovapd %ymm1, (%rdi)
        vzeroupper
        ret
        .cfi_endproc

[Bug c++/66119] [5/6 Regression] in optimization of avx-code

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-05-12
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |jgreenhalgh at gcc dot gnu.org
   Target Milestone|---                         |5.2
            Summary|Regression in optimization  |[5/6 Regression] in
                   |of avx-code                 |optimization of avx-code
     Ever confirmed|0                           |1

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Broken by r217191.

[Bug c++/66119] [5/6 Regression] in optimization of avx-code

[rguenth at gcc dot gnu.org]

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

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.  We expand from

  <bb 2>:
  a$data_13 = MEM[(struct Vec2 *)&a];
  a$32$data_14 = MEM[(struct Vec2 *)&a + 32B];
  b = b;
  v2_15 = MEM[(struct Vec2 *)&b];
  v2$32_16 = MEM[(struct Vec2 *)&b + 32B];
  _7 = a$32$data_14 + v2$32_16;
  _10 = a$data_13 + v2_15;
  MEM[(struct Vec2 *)&<retval>] = _10;
  MEM[(struct Vec2 *)&<retval> + 32B] = _7;
  return <retval>;

in GCC 5 (note the b = b assignment) while 4.9 sees

  <bb 2>:
  a$data_4 = MEM[(struct Vec2 *)&a];
  a$32$data_13 = MEM[(struct Vec2 *)&a + 32B];
  v2_14 = MEM[(struct Vec2 *)&b];
  v2$32_15 = MEM[(struct Vec2 *)&b + 32B];
  _7 = a$32$data_13 + v2$32_15;
  _10 = a$data_4 + v2_14;
  MEM[(struct Vec2 *)&<retval>] = _10;
  MEM[(struct Vec2 *)&<retval> + 32B] = _7;
  return <retval>;

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Ah, it's parameter b assigned to local decl b.

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
And we indeed rely on SRA to copy propagate aggregates.

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[rguenth at gcc dot gnu.org]

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

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed by

Index: toplev.c
===================================================================
--- toplev.c    (revision 223044)
+++ toplev.c    (working copy)
@@ -1311,6 +1311,9 @@ process_options (void)
      so we can correctly initialize debug output.  */
   no_backend = lang_hooks.post_options (&main_input_filename);

+  /* Some machines may reject certain combinations of options.  */
+  targetm.target_option.override ();
+
   /* Set default values for parameters relation to the Scalar Reduction
      of Aggregates passes (SRA and IP-SRA).  We must do this here, rather
      than in opts.c:default_options_optimization as historically these
@@ -1325,9 +1328,6 @@ process_options (void)
      get_move_ratio (false) * UNITS_PER_WORD,
      global_options.x_param_values, global_options_set.x_param_values);

-  /* Some machines may reject certain combinations of options.  */
-  targetm.target_option.override ();
-
   /* Avoid any informative notes in the second run of -fcompare-debug.  */
   if (flag_compare_debug) 
     diagnostic_inhibit_notes (global_dc);

though not sure whether a 2nd maybe_set_param_value will still allow the
target to adjust this default...

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[jgreenhalgh at gcc dot gnu.org]

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

--- Comment #7 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> DEFPARAM (PARAM_SRA_MAX_SCALARIZATION_SIZE_SPEED,
>           "sra-max-scalarization-size-Ospeed",
>           "Maximum size, in storage units, 
> 
> storage units!  But the value seems to be in bits?  It gets used as
> 
>             if (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (var)))
>                 <= max_scalarization_size)
> 

Well, that part should have been covered by:

+  unsigned max_scalarization_size
+    = (optimize_function_for_size_p (cfun)
+       ? PARAM_VALUE (PARAM_SRA_MAX_SCALARIZATION_SIZE_SIZE)
+       : PARAM_VALUE (PARAM_SRA_MAX_SCALARIZATION_SIZE_SPEED))
+      * BITS_PER_UNIT;

>From the original patch, 

> Looks like get_move_ratio returns different things at SRA time (if I re-call
> it)
> and the time it gets invoked in toplev.c.

But, yes these parameters will cause a difference in code generation if
previously MOVE_RATIO could return different values at different times, as it
might well have if target_option_override set up a structure used by
MOVE_RATIO.

The patch I applied carries the hidden assumption that MOVE_RATIO is constant.
Clearly there are a number of situations we might not want that to be true
(say, for switchable targets - which I don't think your patch will help).

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to James Greenhalgh from comment #7)
> (In reply to Richard Biener from comment #5)
> > DEFPARAM (PARAM_SRA_MAX_SCALARIZATION_SIZE_SPEED,
> >           "sra-max-scalarization-size-Ospeed",
> >           "Maximum size, in storage units, 
> > 
> > storage units!  But the value seems to be in bits?  It gets used as
> > 
> >             if (tree_to_uhwi (TYPE_SIZE (TREE_TYPE (var)))
> >                 <= max_scalarization_size)
> > 
> 
> Well, that part should have been covered by:
> 
> +  unsigned max_scalarization_size
> +    = (optimize_function_for_size_p (cfun)
> +	? PARAM_VALUE (PARAM_SRA_MAX_SCALARIZATION_SIZE_SIZE)
> +	: PARAM_VALUE (PARAM_SRA_MAX_SCALARIZATION_SIZE_SPEED))
> +      * BITS_PER_UNIT;
> 
> From the original patch, 
> 
> > Looks like get_move_ratio returns different things at SRA time (if I re-call
> > it)
> > and the time it gets invoked in toplev.c.
> 
> But, yes these parameters will cause a difference in code generation if
> previously MOVE_RATIO could return different values at different times, as
> it might well have if target_option_override set up a structure used by
> MOVE_RATIO.
> 
> The patch I applied carries the hidden assumption that MOVE_RATIO is
> constant. Clearly there are a number of situations we might not want that to
> be true (say, for switchable targets - which I don't think your patch will
> help).

So maybe instead compute that "default" in SRA itself?
>From gcc-bugs-return-486204-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Wed May 13 11:00:34 2015
Return-Path: <gcc-bugs-return-486204-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 107648 invoked by alias); 13 May 2015 11:00:34 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 106983 invoked by uid 55); 13 May 2015 11:00:22 -0000
From: "rguenth at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug tree-optimization/66123] Array of labels as values + ternary operator + pointer arithmetic = internal compiler error
Date: Wed, 13 May 2015 11:00:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: tree-optimization
X-Bugzilla-Version: unknown
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: rguenth at gcc dot gnu.org
X-Bugzilla-Status: ASSIGNED
X-Bugzilla-Resolution:
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: rguenth at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-66123-4-j7vE63bFs9@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-66123-4@http.gcc.gnu.org/bugzilla/>
References: <bug-66123-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2015-05/txt/msg01044.txt.bz2
Content-length: 637

https://gcc.gnu.org/bugzilla/show_bug.cgi?idf123

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Author: rguenth
Date: Wed May 13 10:59:49 2015
New Revision: 223130

URL: https://gcc.gnu.org/viewcvs?rev"3130&root=gcc&view=rev
Log:
2015-05-13  Richard Biener  <rguenther@suse.de>

        PR tree-optimization/66123
        * tree-ssa-dom.c (propagate_rhs_into_lhs): Check if we found
        a taken edge.

        * gcc.dg/torture/pr66123.c: New testcase.

Added:
    trunk/gcc/testsuite/gcc.dg/torture/pr66123.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-dom.c

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
James, can you please address this regression for 5.2.0?

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[jgreenhalgh at gcc dot gnu.org]

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

James Greenhalgh <jgreenhalgh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jgreenhalgh at gcc dot gnu.org

--- Comment #10 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
Yes, sorry for the delay - this completely slipped my mind. I'll start working
on it today.

I think I'm going to go back to the design in some of the earlier proposals for
r217191 and move the call to the hookized MOVE_RATIO in to SRA. It makes the
logic a little messier, as we have to check for parameters, then call off to
the target if they are not set. But that will be the only way we can permit
modifying MOVE_RATIO on a per-function basis. The end result should look
something like
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02267.html

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[jgreenhalgh at gcc dot gnu.org]

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

--- Comment #11 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
There is a patch for this on the Mailing List. Jeff has approved the
code-changes and I need an Ack for the test-case (ping:
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02216.html ).

After that I'll backport it to GCC 5.1.1. Hopefully in time to not block the
release.

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P1                          |P2
      Known to work|                            |4.9.2
      Known to fail|                            |5.1.0

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[jgreenhalgh at gcc dot gnu.org]

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

--- Comment #12 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
Author: jgreenhalgh
Date: Fri Jul  3 09:25:54 2015
New Revision: 225369

URL: https://gcc.gnu.org/viewcvs?rev=225369&root=gcc&view=rev
Log:
[Patch SRA] Fix PR66119 by calling get_move_ratio in SRA

gcc/

        PR tree-optimization/66119
        * toplev.c (process_options): Don't set up default values for
        the sra_max_scalarization_size_{speed,size} parameters.
        * tree-sra (analyze_all_variable_accesses): If no values
        have been set for the sra_max_scalarization_size_{speed,size}
        parameters, call get_move_ratio to get target defaults.

gcc/testsuite/

        PR tree-optimization/66119
        * g++.dg/opt/pr66119.C: New.


Added:
    trunk/gcc/testsuite/g++.dg/opt/pr66119.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/toplev.c
    trunk/gcc/tree-sra.c

[Bug tree-optimization/66119] [5/6 Regression] in optimization of avx-code

[jgreenhalgh at gcc dot gnu.org]

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

James Greenhalgh <jgreenhalgh at gcc dot gnu.org> changed:

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

--- Comment #14 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
Should now be fixed on trunk and gcc-5-branch.