[Bug target/106022] New: [12/13 Regression] Enable vectorizer generates extra load

[Bug target/106022] New: [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

            Bug ID: 106022
           Summary: [12/13 Regression] Enable vectorizer generates extra
                    load
           Product: gcc
           Version: 12.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: hjl.tools at gmail dot com
                CC: crazylht at gmail dot com
  Target Milestone: ---

[hjl@gnu-clx-1 gcc-bisect]$ cat x.c
void foo(char * c) {
    c[0] = 0;
    c[1] = 1;
    c[2] = 2;
    c[3] = 3;
}
[hjl@gnu-clx-1 gcc-bisect]$ gcc -O3 x.c -S
[hjl@gnu-clx-1 gcc-bisect]$ cat x.s 
        .file   "x.c"
        .text
        .p2align 4
        .globl  foo
        .type   foo, @function
foo:
.LFB0:
        .cfi_startproc
        movl    .LC0(%rip), %eax
        movl    %eax, (%rdi)
        ret
        .cfi_endproc
.LFE0:
        .size   foo, .-foo
        .section        .rodata.cst4,"aM",@progbits,4
        .align 4
.LC0:
        .byte   0
        .byte   1
        .byte   2
        .byte   3
        .ident  "GCC: (GNU) 12.1.1 20220507 (Red Hat 12.1.1-1)"
        .section        .note.GNU-stack,"",@progbits
[hjl@gnu-clx-1 gcc-bisect]$ gcc -O3 x.c -S -fno-tree-vectorize
[hjl@gnu-clx-1 gcc-bisect]$ cat x.s
        .file   "x.c"
        .text
        .p2align 4
        .globl  foo
        .type   foo, @function
foo:
.LFB0:
        .cfi_startproc
        movl    $50462976, (%rdi)
        ret
        .cfi_endproc
.LFE0:
        .size   foo, .-foo
        .ident  "GCC: (GNU) 12.1.1 20220507 (Red Hat 12.1.1-1)"
        .section        .note.GNU-stack,"",@progbits
[hjl@gnu-clx-1 gcc-bisect]$ /usr/gcc-11.2.1-x32/bin/gcc -S -O3 x.c 
[hjl@gnu-clx-1 gcc-bisect]$ cat x.s
        .file   "x.c"
        .text
        .p2align 4
        .globl  foo
        .type   foo, @function
foo:
.LFB0:
        .cfi_startproc
        movl    $50462976, (%rdi)
        ret
        .cfi_endproc
.LFE0:
        .size   foo, .-foo
        .ident  "GCC: (GNU) 11.2.1 20220118"
        .section        .note.GNU-stack,"",@progbits
[hjl@gnu-clx-1 gcc-bisect]$

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

--- Comment #1 from H.J. Lu <hjl.tools at gmail dot com> ---
SLP thinks that it needs 4 stores to store 4 bytes of integer constant.
But it takes only 1 4-byte store.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[crazylht at gmail dot com]

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

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to H.J. Lu from comment #1)
> SLP thinks that it needs 4 stores to store 4 bytes of integer constant.
> But it takes only 1 4-byte store.

I think we can simplify that at combine.

Failed to match this instruction:
(set (mem:V4QI (reg:DI 84) [0 MEM <vector(4) char> [(char *)c_2(D)]+0 S4 A8])
    (const_vector:V4QI [
            (const_int 0 [0])
            (const_int 1 [0x1])
            (const_int 2 [0x2])
            (const_int 3 [0x3])
        ]))

x86 at most supports mov imm64, m64, so v2qi/v4qi/v8qi/v2hi/v4hi/v2hf/v4hf
const_vector stores can be simplified to just mov imm64/imm32/imm16,
m64/m32/m16.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.2
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-06-20
             Status|UNCONFIRMED                 |NEW

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
We already put the const_vector into the constant pool at RTL expansion time,
possibly because the move patterns do not allow vector constants fitting into
{SI,DI}mode?

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[rguenth at gcc dot gnu.org]

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #1)
> SLP thinks that it needs 4 stores to store 4 bytes of integer constant.
> But it takes only 1 4-byte store.

Yes, SLP looks at the GIMPLE IL, if you disable store-merging you'll get
4 stores.  SLP doesn't anticipate any optimizations that follow it.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[crazylht at gmail dot com]

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

--- Comment #5 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to Richard Biener from comment #3)
> We already put the const_vector into the constant pool at RTL expansion time,
> possibly because the move patterns do not allow vector constants fitting into
> {SI,DI}mode?

Good idea, it maybe better to support it in the move patterns.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

--- Comment #6 from H.J. Lu <hjl.tools at gmail dot com> ---
Created attachment 53169
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53169&action=edit
A patch

This patch multiplies the vector store cost by the number of scalar elements in
a word to properly compare scalar store cost against vector store cost.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

--- Comment #7 from H.J. Lu <hjl.tools at gmail dot com> ---
Created attachment 53171
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53171&action=edit
The v2 patch

Handle vector store.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[rguenth at gcc dot gnu.org]

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

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #6)
> Created attachment 53169 [details]
> A patch
> 
> This patch multiplies the vector store cost by the number of scalar elements
> in
> a word to properly compare scalar store cost against vector store cost.

But that's not "properly" but "wrong" ...

Note we already cost the vector load from the constant pool so the vector
side costing is correct.

What's eventually imprecise is the scalar cost where you could anticipate
store merging, but adjusting the vector cost side is just wrong.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

--- Comment #9 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Richard Biener from comment #8)
> (In reply to H.J. Lu from comment #6)
> > Created attachment 53169 [details]
> > A patch
> > 
> > This patch multiplies the vector store cost by the number of scalar elements
> > in
> > a word to properly compare scalar store cost against vector store cost.
> 
> But that's not "properly" but "wrong" ...
> 
> Note we already cost the vector load from the constant pool so the vector
> side costing is correct.
> 
> What's eventually imprecise is the scalar cost where you could anticipate
> store merging, but adjusting the vector cost side is just wrong.

I tried to adjust the scalar cost.  When the scalar cost of storing a byte
is 6, dividing it by 8 (the number of scalar elements in a word) becomes 0.
Will it work?

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53171|0                           |1
        is obsolete|                            |

--- Comment #10 from H.J. Lu <hjl.tools at gmail dot com> ---
Created attachment 53186
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53186&action=edit
The v3 patch

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to H.J. Lu from comment #9)
> (In reply to Richard Biener from comment #8)
> > (In reply to H.J. Lu from comment #6)
> > > Created attachment 53169 [details]
> > > A patch
> > > 
> > > This patch multiplies the vector store cost by the number of scalar elements
> > > in
> > > a word to properly compare scalar store cost against vector store cost.
> > 
> > But that's not "properly" but "wrong" ...
> > 
> > Note we already cost the vector load from the constant pool so the vector
> > side costing is correct.
> > 
> > What's eventually imprecise is the scalar cost where you could anticipate
> > store merging, but adjusting the vector cost side is just wrong.
> 
> I tried to adjust the scalar cost.  When the scalar cost of storing a byte
> is 6, dividing it by 8 (the number of scalar elements in a word) becomes 0.
> Will it work?

No, I think you would need to pattern match an actual store sequence,
for example by looking at

 if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
     && pow2p_hwi (DR_GROUP_STORE_COUNT (stmt_info)))
   /* cost a possibly merged store only once (but with larger mode?) */
   if (DR_GROUP_FIRST_ELEMENT (stmt_info) == stmt_info)
     ...

So costing the whole sequence of scalar stores a single time, with
adjusted mode.

store-merging also handles non-QImode stores btw.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

--- Comment #12 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Richard Biener from comment #11)
> (In reply to H.J. Lu from comment #9)
> > (In reply to Richard Biener from comment #8)
> > > (In reply to H.J. Lu from comment #6)
> > > > Created attachment 53169 [details]
> > > > A patch
> > > > 
> > > > This patch multiplies the vector store cost by the number of scalar elements
> > > > in
> > > > a word to properly compare scalar store cost against vector store cost.
> > > 
> > > But that's not "properly" but "wrong" ...
> > > 
> > > Note we already cost the vector load from the constant pool so the vector
> > > side costing is correct.
> > > 
> > > What's eventually imprecise is the scalar cost where you could anticipate
> > > store merging, but adjusting the vector cost side is just wrong.
> > 
> > I tried to adjust the scalar cost.  When the scalar cost of storing a byte
> > is 6, dividing it by 8 (the number of scalar elements in a word) becomes 0.
> > Will it work?
> 
> No, I think you would need to pattern match an actual store sequence,
> for example by looking at
> 
>  if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
>      && pow2p_hwi (DR_GROUP_STORE_COUNT (stmt_info)))
>    /* cost a possibly merged store only once (but with larger mode?) */
>    if (DR_GROUP_FIRST_ELEMENT (stmt_info) == stmt_info)
>      ...

The information aren't available in add_stmt_cost.  I will
count number of scalar stores and vector stores.  Then I will
compare them in finish_cost.

> So costing the whole sequence of scalar stores a single time, with
> adjusted mode.
> 
> store-merging also handles non-QImode stores btw.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[rguenther at suse dot de]

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

--- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> ---
On Fri, 24 Jun 2022, hjl.tools at gmail dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106022
> 
> --- Comment #12 from H.J. Lu <hjl.tools at gmail dot com> ---
> (In reply to Richard Biener from comment #11)
> > No, I think you would need to pattern match an actual store sequence,
> > for example by looking at
> > 
> >  if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
> >      && pow2p_hwi (DR_GROUP_STORE_COUNT (stmt_info)))
> >    /* cost a possibly merged store only once (but with larger mode?) */
> >    if (DR_GROUP_FIRST_ELEMENT (stmt_info) == stmt_info)
> >      ...
> 
> The information aren't available in add_stmt_cost.

They should be ...

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

--- Comment #14 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to rguenther@suse.de from comment #13)
> On Fri, 24 Jun 2022, hjl.tools at gmail dot com wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106022
> > 
> > --- Comment #12 from H.J. Lu <hjl.tools at gmail dot com> ---
> > (In reply to Richard Biener from comment #11)
> > > No, I think you would need to pattern match an actual store sequence,
> > > for example by looking at
> > > 
> > >  if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
> > >      && pow2p_hwi (DR_GROUP_STORE_COUNT (stmt_info)))
> > >    /* cost a possibly merged store only once (but with larger mode?) */
> > >    if (DR_GROUP_FIRST_ELEMENT (stmt_info) == stmt_info)
> > >      ...
> > 
> > The information aren't available in add_stmt_cost.
> 
> They should be ...

I meant that DR_GROUP_STORE_COUNT (stmt_info) was zero.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[rguenther at suse dot de]

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

--- Comment #15 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 27 Jun 2022, hjl.tools at gmail dot com wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106022
> 
> --- Comment #14 from H.J. Lu <hjl.tools at gmail dot com> ---
> (In reply to rguenther@suse.de from comment #13)
> > On Fri, 24 Jun 2022, hjl.tools at gmail dot com wrote:
> > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106022
> > > 
> > > --- Comment #12 from H.J. Lu <hjl.tools at gmail dot com> ---
> > > (In reply to Richard Biener from comment #11)
> > > > No, I think you would need to pattern match an actual store sequence,
> > > > for example by looking at
> > > > 
> > > >  if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
> > > >      && pow2p_hwi (DR_GROUP_STORE_COUNT (stmt_info)))
> > > >    /* cost a possibly merged store only once (but with larger mode?) */
> > > >    if (DR_GROUP_FIRST_ELEMENT (stmt_info) == stmt_info)
> > > >      ...
> > > 
> > > The information aren't available in add_stmt_cost.
> > 
> > They should be ...
> 
> I meant that DR_GROUP_STORE_COUNT (stmt_info) was zero.

Ah, OK - use DR_GROUP_SIZE then.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[hjl.tools at gmail dot com]

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

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53186|0                           |1
        is obsolete|                            |

--- Comment #16 from H.J. Lu <hjl.tools at gmail dot com> ---
Created attachment 53227
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53227&action=edit
The v4 patch

Here is a different approach by supporting 2/4/8 byte constant vector stores.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[crazylht at gmail dot com]

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

--- Comment #17 from Hongtao.liu <crazylht at gmail dot com> ---
(In reply to H.J. Lu from comment #16)
> Created attachment 53227 [details]
> The v4 patch
> 
> Here is a different approach by supporting 2/4/8 byte constant vector stores.

It LGTM.

[Bug target/106022] [12/13 Regression] Enable vectorizer generates extra load

[cvs-commit at gcc dot gnu.org]

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

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by H.J. Lu <hjl@gcc.gnu.org>:

https://gcc.gnu.org/g:f3a5e75cb66dc96efca7d494fe6060746c88acb1

commit r13-1415-gf3a5e75cb66dc96efca7d494fe6060746c88acb1
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Fri Jun 17 17:02:05 2022 -0700

    x86: Support 2/4/8 byte constant vector stores

    1. Add a predicate for constant vectors which can be converted to integer
    constants suitable for constant integer stores.  For a 8-byte constant
    vector, the converted 64-bit integer must be valid for store with 64-bit
    immediate, which is a 64-bit integer sign-extended from a 32-bit integer.
    2. Add a new pattern to allow 2-byte, 4-byte and 8-byte constant vector
    stores, like

    (set (mem:V2HI (reg:DI 84))
         (const_vector:V2HI [(const_int 0 [0]) (const_int 1 [0x1])]))

    3. After reload, convert constant vector stores to constant integer
    stores, like

    (set (mem:SI (reg:DI 5 di [84]))
         (const_int 65536 [0x10000]))

    For

    void
    foo (short * c)
    {
      c[0] = 0;
      c[1] = 1;
    }

    it generates

            movl    $65536, (%rdi)

    instead of

            movl    .LC0(%rip), %eax
            movl    %eax, (%rdi)

    gcc/

            PR target/106022
            * config/i386/i386-protos.h (ix86_convert_const_vector_to_integer):
            New.
            * config/i386/i386.cc (ix86_convert_const_vector_to_integer):
            New.
            * config/i386/mmx.md (V_16_32_64): New.
            (*mov<mode>_imm): New patterns for stores with 16-bit, 32-bit
            and 64-bit constant vector.
            * config/i386/predicates.md (x86_64_const_vector_operand): New.

    gcc/testsuite/

            PR target/106022
            * gcc.target/i386/pr106022-1.c: New test.
            * gcc.target/i386/pr106022-2.c: Likewise.
            * gcc.target/i386/pr106022-3.c: Likewise.
            * gcc.target/i386/pr106022-4.c: Likewise.

[Bug target/106022] [12 Regression] Enable vectorizer generates extra load

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12/13 Regression] Enable   |[12 Regression] Enable
                   |vectorizer generates extra  |vectorizer generates extra
                   |load                        |load
           Priority|P3                          |P2
      Known to work|                            |13.0

--- Comment #19 from Richard Biener <rguenth at gcc dot gnu.org> ---
I assume fixed on trunk now.

[Bug target/106022] [12 Regression] Enable vectorizer generates extra load

[rguenth at gcc dot gnu.org]

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

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
Probably OK to backport for 12.2 (but no later)

[Bug target/106022] [12 Regression] Enable vectorizer generates extra load

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |570070308 at qq dot com

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
*** Bug 108441 has been marked as a duplicate of this bug. ***

[Bug target/106022] [12 Regression] Enable vectorizer generates extra load

[rguenth at gcc dot gnu.org]

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

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

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

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