[Bug target/104271] New: 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[Bug target/104271] New: 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[jamborm at gcc dot gnu.org]

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

            Bug ID: 104271
           Summary: 538.imagick_r run-time at -Ofast -march=native
                    regressed by 26% on Intel Cascade Lake server CPU
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jamborm at gcc dot gnu.org
                CC: liuhongt at gcc dot gnu.org
            Blocks: 26163
  Target Milestone: ---
              Host: x86_64-linux
            Target: x86_64-linux

On our Intel Cascade Lake server CPU, the benchmark 538.imagick_r from
the SPEC 2017 FPrate suite is 26% slower when built with GCC 12 using
options -Ofast -march=native than when built with GCC 11 with the same
options.

I have bisected the issue to r12-2549-g872da9a6f664a0:

  872da9a6f664a06d73c987aa0cb2e5b830158a10 is the first bad commit
  commit 872da9a6f664a06d73c987aa0cb2e5b830158a10
  Author: liuhongt <hongtao.liu@intel.com>
  Date:   Fri Mar 26 10:56:47 2021 +0800

    Add the member integer_to_sse to processor_cost as a cost simulation for
movd/pinsrd. It will be used to calculate the cost of vec_construct.

    gcc/ChangeLog: 

            PR target/99881
            * config/i386/i386.h (processor_costs): Add new member
            integer_to_sse.
            * config/i386/x86-tune-costs.h (ix86_size_cost, i386_cost,
            i486_cost, pentium_cost, lakemont_cost, pentiumpro_cost,
            geode_cost, k6_cost, athlon_cost, k8_cost, amdfam10_cost,
            bdver_cost, znver1_cost, znver2_cost, znver3_cost,
            btver1_cost, btver2_cost, btver3_cost, pentium4_cost,
            nocona_cost, atom_cost, atom_cost, slm_cost, intel_cost,
            generic_cost, core_cost): Initialize integer_to_sse same value
            as sse_op.
            (skylake_cost): Initialize integer_to_sse twice as much as sse_op.
            * config/i386/i386.c (ix86_builtin_vectorization_cost):
            Use integer_to_sse instead of sse_op to calculate the cost of
            vec_construct.


It is very likely that the problem is a load-to-store-forwarding stall
issue and so related to PR 80689 but the commit makes it either more
frequent or much worse.

Sorry for reporting this so late, unfortunately we do not benchmark
this class of CPUs periodically.  We do not see the problem on any of
the AMD Zens or Intel Kabylake (client) CPUs.


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=26163
[Bug 26163] [meta-bug] missed optimization in SPEC (2k17, 2k and 2k6 and 95)

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
   Target Milestone|---                         |12.0
            Summary|538.imagick_r run-time at   |[12 Regression]
                   |-Ofast -march=native        |538.imagick_r run-time at
                   |regressed by 26% on Intel   |-Ofast -march=native
                   |Cascade Lake server CPU     |regressed by 26% on Intel
                   |                            |Cascade Lake server CPU

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[marxin at gcc dot gnu.org]

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

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-01-31
             Status|UNCONFIRMED                 |NEW
                 CC|                            |marxin at gcc dot gnu.org

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[crazylht at gmail dot com]

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

--- Comment #1 from Hongtao.liu <crazylht at gmail dot com> ---
I think this patch has already been reverted by
r12-3011-g1db70e61a92978377a648bbd90e383859fc0126b.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[jamborm at gcc dot gnu.org]

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

--- Comment #2 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Hongtao.liu from comment #1)
> I think this patch has already been reverted by
> r12-3011-g1db70e61a92978377a648bbd90e383859fc0126b.

Unfortunately that revision does not help.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[crazylht at gmail dot com]

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

--- Comment #3 from Hongtao.liu <crazylht at gmail dot com> ---
I think it's related to r12-2666 which caused load-to-store-forwarding stall
issue, and we're working on pr103734, try to improve ipa-inline.

w -param=inline-min-speedup=3, we observe both performance improvement and
codesize decrease.

Plan to land on GCC13.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |WAITING

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Does this still happen after the last fiddling?

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[jamborm at gcc dot gnu.org]

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

--- Comment #5 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #4)
> Does this still happen after the last fiddling?

Unfortunately yes, the run-time is still 27% worse than when compiled with the
commit previous to the one identified in the bug summary (I tried with
e6a3991ea15).

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[lili.cui at intel dot com]

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

--- Comment #6 from cuilili <lili.cui at intel dot com> ---
I created a patch to fix this regression. The patch is under performance
testing. Will sent it out later.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[lili.cui at intel dot com]

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

--- Comment #7 from cuilili <lili.cui at intel dot com> ---
Created attachment 52706
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52706&action=edit
Add a heuristic for eliminate redundant load and store in inline pass.

Hi Richard,

Could you help take a look? This is my first time adding code in mid-end, hope
you can give me some advice, thank you!

I add a INLINE_HINT_eliminate_load_and_store hint in to inline pass. when
callee's memory access is caller's local memory parameter and access size is
greater than the target threshold, we will enable the hint. with the hint,
inlining_insns_auto will enlarge the bound. The target hook is only enabled for
x86 now.

With the patch applied
Icelake server: 538.imagic_r get 15.18% improvement for multicopy and 40.78%
improvement for single copy with no measurable changes for other benchmarks.

Casecadelake: 538.imagic_r get 12.4% improvement for multicopy with and code
size increased by 0.4%. With no measurable changes for other benchmarks.

Znver3 server: 538.imagic_r get 9.6% improvement for multicopy with and code
size increased by 0.5%. With no measurable changes for other benchmarks.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |jamborm at gcc dot gnu.org

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to cuilili from comment #7)
> Created attachment 52706 [details]
> Add a heuristic for eliminate redundant load and store in inline pass.
> 
> Hi Richard,
> 
> Could you help take a look? This is my first time adding code in mid-end,
> hope you can give me some advice, thank you!
> 
> I add a INLINE_HINT_eliminate_load_and_store hint in to inline pass. when
> callee's memory access is caller's local memory parameter and access size is
> greater than the target threshold, we will enable the hint. with the hint,
> inlining_insns_auto will enlarge the bound. The target hook is only enabled
> for x86 now.
> 
> With the patch applied
> Icelake server: 538.imagic_r get 15.18% improvement for multicopy and 40.78%
> improvement for single copy with no measurable changes for other benchmarks.
> 
> Casecadelake: 538.imagic_r get 12.4% improvement for multicopy with and code
> size increased by 0.4%. With no measurable changes for other benchmarks.
> 
> Znver3 server: 538.imagic_r get 9.6% improvement for multicopy with and code
> size increased by 0.5%. With no measurable changes for other benchmarks.

It's an interesting idea, note Honza knows better about IPA modref and
inlining than me.  What I doubt is that you can directly use IPA modref
info to determine whether inlining will likely elide a store/load pair
since IIRC the modref info is for the whole function.

IPA SRA might perform the kind of analysis that is contained to the
call context and that might be available here already (and eventually
even IPA SRA considers passing the stored/loaded values by value?)

But yes, having a stream of up to N (independent?) stores before each call
plus a stream of up to M (independent hoistable to function start?) loads
at each function start would make such analysis possible.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[lili.cui at intel dot com]

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

--- Comment #9 from cuilili <lili.cui at intel dot com> ---
Really appreciate for your reply, I debugged SRA pass with the small testcase
and found that SRA dose not handle this situation.

SRA cannot split callee's first parameter for "Do not decompose non-BLKmode
parameters in a way that would create a BLKmode parameter. Especially for
pass-by-reference (hence, pointer type parameters), it's not worth it."

Before inline:
For caller 
store-1 :   128 bits store of struct "a" (it is an implicit store during IPA
pass, the store can only be found after a certain pass.)
For callee
load-1 :    128 bits load of struct "a" for operation "c->a=(*a)"
store-2:    128 bits store of struct "c->a" for operation "c->a=(*a)" 
load-2 :    4 * 32 bits load for c->a.f1, c->a.f2, c->a.f3 and c->a.f4.
(because the store-2 using vector register to store, we cannot use the register
directly here.) 

After inline:
For caller
None.
For callee
store-2 :  128 bits store of struct c->a for operation "c->a=(*a)"

--------------------------------------------------------
int callee (struct A *a, struct C *c)
{
  c->a=(*a);   
  if ((c->b + 7) & 17)
    {
      c->a.f1 = c->a.f2 + c->a.f3;
      c->a.f2 = c->a.f2 - c->a.f3;
      c->a.f3 = c->a.f2 + c->a.f3;
      c->a.f4 = c->a.f2 - c->a.f3;
      c->b = c->a.f2 + c->a.f4;
      return 0;
    }
  return 1;
}

int caller (int d, struct C *c)
{
  struct A a;
  a.f1 = 1 + d;
  a.f2 = 2;
  a.f3 = 12 + d;
  a.f4 = 68 + d;
  if (d > 0)
    return callee (&a, c);
  else
    return 1;
}
-------------------------------------------------
In 538.imagic_r（c_ray also has the similar code）, if we inline the hot
function, the redundant store and load structure's size is 256 bits (4 elements
of size 64 bits), which can eliminates one 256-bit store, one 256-bit load, and
four 64-bit loads.
can I do it like this? Computes the total size of all callee arguments that can
eliminate redundant loads and stores. Thanks!

[Bug target/104271] [12/13 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.0                        |12.2

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 12.1 is being released, retargeting bugs to GCC 12.2.

[Bug target/104271] [12/13 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[lili.cui at intel dot com]

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

--- Comment #12 from cuilili <lili.cui at intel dot com> ---
This regression caused by the store forwarding issue, we eliminate the
redundant two pairs of loads and stores which have store forwarding issue by
inlining. 

This regression has been fixed by 

https://gcc.gnu.org/g:1b9a5cc9ec08e9f239dd2096edcc447b7a72f64a

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[lili.cui at intel dot com]

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

--- Comment #14 from cuilili <lili.cui at intel dot com> ---
This regression has been fixed with the commit below and we can close this
ticket.

https://gcc.gnu.org/g:1b9a5cc9ec08e9f239dd2096edcc447b7a72f64a

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[sjames at gcc dot gnu.org]

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

Sam James <sjames at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED
                 CC|                            |sjames at gcc dot gnu.org

--- Comment #15 from Sam James <sjames at gcc dot gnu.org> ---
(In reply to cuilili from comment #14)
> This regression has been fixed with the commit below and we can close this
> ticket.
>  
> https://gcc.gnu.org/g:1b9a5cc9ec08e9f239dd2096edcc447b7a72f64a

Thanks. Do you have a gcc.gnu.org email address? If so, if you change your
Bugzilla to that, you should have permissions to modify bugs yourself.

[Bug target/104271] [12 Regression] 538.imagick_r run-time at -Ofast -march=native regressed by 26% on Intel Cascade Lake server CPU

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.4                        |12.3