[Bug target/109797] New: 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[Bug target/109797] New: 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[jamborm at gcc dot gnu.org]

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

            Bug ID: 109797
           Summary: 456.hmmer compiled with -O2 -flto regressed by 15% on
                    AMD zen3 between r14-487-g6f18f344338b37 and
                    r14-540-gb7fe38c14e5f1b
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jamborm at gcc dot gnu.org
            Blocks: 26163
  Target Milestone: ---

The SPEC 2006 benchmark compiled with -O2 and -flto regressed by 14% on Zen3 as
can be seen at:

https://lnt.opensuse.org/db_default/v4/SPEC/graph?plot.0=469.180.0

Unfortunately I cannot see a similar issue on other machines, not even on zen2
or the Intel x86_64 we run benchmarks on.  (That is why I chose the component
to be target even though it is little more than a wild guess.)


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/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Maybe:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=919642fa4b2bc4c32910336dd200d53766801c80

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=109690

--- Comment #2 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Probably a good guess, on a different zen3 based machine, the revision
r14-493-g919642fa4b2bc4 makes the run-time of 456.hmmer grow by 10%

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[ubizjak at gmail dot com]

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

--- Comment #3 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Andrew Pinski from comment #1)
> Maybe:
> https://gcc.gnu.org/git/?p=gcc.git;a=commit;
> h=919642fa4b2bc4c32910336dd200d53766801c80

Is this with -msse4? In case of TARGET_SSE4_1 the revision just emits PMULLD,
otherwise it emits PMULUDQ with corresponding PUNPCKLDQ and PSHUFD shuffles.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|                            |x86_64-*-*

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
No, it's indeed plain -O2 with the default architecture level, thus SSE2 only.

For the case of "complex" expansions we might want to bite the bullet and
fix the GIGO vectorizer cost modeling in the target.  add_stmt_cost should
use ix86_multiplication_cost here passing down V2SImode.  There's cases
for various integer multiplication emulations but your patch didn't amend
this function for the new V2SImode emulation w/o SSE4.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[ubizjak at gmail dot com]

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

--- Comment #5 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Richard Biener from comment #4)
> No, it's indeed plain -O2 with the default architecture level, thus SSE2
> only.
> 
> For the case of "complex" expansions we might want to bite the bullet and
> fix the GIGO vectorizer cost modeling in the target.  add_stmt_cost should
> use ix86_multiplication_cost here passing down V2SImode.  There's cases
> for various integer multiplication emulations but your patch didn't amend
> this function for the new V2SImode emulation w/o SSE4.

Maybe the pattern should only be enabled for TARGET_SSE4_1, where it is
implemented with native instruction. Using PMULUDQ, shuffling arguments and the
result around looks like it will never be a win for V2SI, while it is
worthwhile for V4SI, where the approach is taken from.

The pattern was introduced as a partial fix for PR109690.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[ubizjak at gmail dot com]

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

--- Comment #6 from Uroš Bizjak <ubizjak at gmail dot com> ---
Created attachment 55045
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55045&action=edit
Proposed patch

Proposed patch to fix the ix86_multiplication_cost for non-SSE4 V2SI emulation.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[ubizjak at gmail dot com]

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

--- Comment #7 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Uroš Bizjak from comment #6)
> Created attachment 55045 [details]
> Proposed patch
> 
> Proposed patch to fix the ix86_multiplication_cost for non-SSE4 V2SI
> emulation.

Martin, does this patch fix the runtime regression?

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[jamborm at gcc dot gnu.org]

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

--- Comment #8 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Uroš Bizjak from comment #7)
> 
> Martin, does this patch fix the runtime regression?

No, unfortunately it does not.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[ubizjak at gmail dot com]

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

--- Comment #9 from Uroš Bizjak <ubizjak at gmail dot com> ---
Created attachment 55057
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55057&action=edit
Patch to enable mulv2si for TARGET_SSE4_1 only

The alternative approach is to enable mulv2si for TARGET_SSE4_1 only. This will
avoid vectorization with emulated mul sequence for TARGET_SSE2, but until there
is more precise vectorization cost estimation, this is IMO the best approach to
avoid runtime regression for now.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[jamborm at gcc dot gnu.org]

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

--- Comment #10 from Martin Jambor <jamborm at gcc dot gnu.org> ---
The patch from comment #9 does fix the regression.  Thanks.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Uros Bizjak <uros@gcc.gnu.org>:

https://gcc.gnu.org/g:75aa95694ed9bab50b5e152f7a755eb5f5e8014e

commit r14-794-g75aa95694ed9bab50b5e152f7a755eb5f5e8014e
Author: Uros Bizjak <ubizjak@gmail.com>
Date:   Fri May 12 18:37:13 2023 +0200

    i386: Remove mulv2si emulated sequence for TARGET_SSE2 [PR109797]

    Remove mulv2si emulated sequence for TARGET_SSE2 and enable
    only native PMULLD instruction for TARGET_SSE4_1.  Ideally, the
    vectorization for TARGET_SSE2 should depend on more precise cost
    estimation (the PR contains patch for ix86_multiplication_cost),
    but even with patched cost function the runtime regression
    was not fixed.

            PR target/109797

    gcc/ChangeLog:

            * config/i386/mmx.md (mulv2si3): Remove expander.
            (mulv2si3): Rename insn pattern from *mulv2si.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[ubizjak at gmail dot com]

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

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
   Target Milestone|---                         |14.0
         Resolution|---                         |FIXED
           Assignee|unassigned at gcc dot gnu.org      |ubizjak at gmail dot com

--- Comment #12 from Uroš Bizjak <ubizjak at gmail dot com> ---
Fixed.

[Bug target/109797] 456.hmmer compiled with -O2 -flto regressed by 15% on AMD zen3 between r14-487-g6f18f344338b37 and r14-540-gb7fe38c14e5f1b

[rguenth at gcc dot gnu.org]

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

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Uroš Bizjak from comment #5)
> (In reply to Richard Biener from comment #4)
> > No, it's indeed plain -O2 with the default architecture level, thus SSE2
> > only.
> > 
> > For the case of "complex" expansions we might want to bite the bullet and
> > fix the GIGO vectorizer cost modeling in the target.  add_stmt_cost should
> > use ix86_multiplication_cost here passing down V2SImode.  There's cases
> > for various integer multiplication emulations but your patch didn't amend
> > this function for the new V2SImode emulation w/o SSE4.
> 
> Maybe the pattern should only be enabled for TARGET_SSE4_1, where it is
> implemented with native instruction. Using PMULUDQ, shuffling arguments and
> the result around looks like it will never be a win for V2SI, while it is
> worthwhile for V4SI, where the approach is taken from.

Yeah, in theory, if the vectorizer would consider keeping some ops scalar
it would be costed against two element extracts, two mulsd, and a build
of a V2SI vector again.

Note even locally "bad" vectorizations can pay off if they are required
to vectorize a larger sequence.  But we are quite bad at estimating overall
latency (and resource utilization).  What also often happens is that
SSE op "badness" is marginal compared to the comperatively very high costs
of memory ops (I think their cost is overly high when from L1).  Esp. as
we consider the costs to be "latency" we probably should cost stores as
zero because we are not interested in when they complete.  IIRC we
consider a scalar store and a vector store to cost 12, so we "win" 12 by
eliding a single store.  In reality if it takes 1 cycle to "issue" the
store we should have them cost 1 so we get a slight win but not 12?

The story is different for loads of course but even there 2 parallel
scalar loads vs. 1 vector load isn't 12 + 12 latency vs 12 but rather
12 + 1(?) vs 12.  But since we cost the scalar and vector IL separately
this is somewhat difficult to assess.  Maybe it would be easier to
compute a (local) "relative" cost, presenting related scalar and vector
stmts to the hook at the same time (or have the hook "guess" the scalar
ops - a good guess is usually straight-forward of course).

> The pattern was introduced as a partial fix for PR109690.