[Bug target/54201] New: XMM constant duplicated

[Bug target/54201] New: XMM constant duplicated

[cesarb at cesarb dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

             Bug #: 54201
           Summary: XMM constant duplicated
    Classification: Unclassified
           Product: gcc
           Version: 4.6.3
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: cesarb@cesarb.net


Created attachment 27960
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27960
Reduced testcase

When compiling the following small function (command line used "gcc -std=c99
-Wall -Wextra -Werror -O3 -save-temps -c pcmpeqb.c"):

#include <emmintrin.h>

__m128i test(__m128i value)
{
    __m128i mask = _mm_set1_epi8(1);
    return _mm_cmpeq_epi8(_mm_and_si128(value, mask), mask);
}

gcc creates two identical copies of the "mask" constant, one with repeated
.quad, the other with repeated .byte:

[...]
    pand    .LC0(%rip), %xmm0
    pcmpeqb    .LC1(%rip), %xmm0
[...]
    .align 16
.LC0:
    .quad    72340172838076673
    .quad    72340172838076673
    .align 16
.LC1:
    .byte    1
    .byte    1
[...]

It should create and use only one copy of the constant.

With more complex code in which "test" is a inline function called several
times, gcc uses two registers instead of one for the constant; it seems it
cannot see it is the same value, even though it is explicitly the same value in
the source code.

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/libexec/gcc/x86_64-redhat-linux/4.6.3/lto-wrapper
Target: x86_64-redhat-linux
Configured with: ../configure --prefix=/usr --mandir=/usr/share/man
--infodir=/usr/share/info --with-bugurl=http://bugzilla.redhat.com/bugzilla
--enable-bootstrap --enable-shared --enable-threads=posix
--enable-checking=release --with-system-zlib --enable-__cxa_atexit
--disable-libunwind-exceptions --enable-gnu-unique-object
--enable-linker-build-id
--enable-languages=c,c++,objc,obj-c++,java,fortran,ada,go,lto --enable-plugin
--enable-java-awt=gtk --disable-dssi
--with-java-home=/usr/lib/jvm/java-1.5.0-gcj-1.5.0.0/jre
--enable-libgcj-multifile --enable-java-maintainer-mode
--with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libjava-multilib
--with-ppl --with-cloog --with-tune=generic --with-arch_32=i686
--build=x86_64-redhat-linux
Thread model: posix
gcc version 4.6.3 20120306 (Red Hat 4.6.3-2) (GCC)

[Bug target/54201] XMM constant duplicated

[cesarb at cesarb dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

--- Comment #1 from Cesar Eduardo Barros <cesarb at cesarb dot net> 2012-08-08 11:32:37 UTC ---
Created attachment 27961
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27961
Assembly output

[Bug middle-end/54201] XMM constant duplicated

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-08-08
          Component|target                      |middle-end
     Ever Confirmed|0                           |1
      Known to fail|                            |4.8.0

--- Comment #2 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-08 11:53:16 UTC ---
That's because on the GIMPLE level we are type correct and see

test (__m128i value)
{
  <bb 2>:
  <anon>_3 = __builtin_ia32_pand128 (value_1(D), { 72340172838076673,
72340172838076673 });
  <anon>_4 = VIEW_CONVERT_EXPR<vector(16) char>(<anon>_3);
  <anon>_5 = __builtin_ia32_pcmpeqb128 (<anon>_4, { 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1 });
  <anon>_6 = VIEW_CONVERT_EXPR<__m128i>(<anon>_5);
  return <anon>_6;

I don't think we have too fancy constant pool code to allow sharing of
bit-identical values.

Confirmed.  middle-end, constant pool handling in varasm.c.

[Bug middle-end/54201] XMM constant duplicated

[cesarb at cesarb dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

--- Comment #3 from Cesar Eduardo Barros <cesarb at cesarb dot net> 2012-08-08 23:28:52 UTC ---
Created attachment 27964
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27964
Another testcase

Another testcase. This one also shows it using two registers instead of one.

        movdqa  .LC0(%rip), %xmm3
        pand    %xmm3, %xmm0
        movdqa  .LC1(%rip), %xmm2
        pcmpeqb %xmm2, %xmm0
        pand    %xmm1, %xmm0
        pand    %xmm3, %xmm0
        pcmpeqb %xmm2, %xmm0

%xmm2 and %xmm3 have the same value, so it is increasing the register pressure
unnecessarily.

[Bug middle-end/54201] XMM constant duplicated

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-09 09:49:49 UTC ---
Mine for now.

[Bug middle-end/54201] XMM constant duplicated

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

--- Comment #5 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-09 10:21:57 UTC ---
Created attachment 27965
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27965
idea

Uh, the constant pool is filled via force_const_mem and I thought about
beefing up the wrong routines ... unfortunately we don't have a
native_encode_expr for RTXen.

See attachment for the idea which also would allow sharing parts of constants
(if a larger one happens to be output first).

[Bug middle-end/54201] XMM constant duplicated

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
         AssignedTo|rguenth at gcc dot gnu.org  |unassigned at gcc dot
                   |                            |gnu.org

--- Comment #6 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-13 12:42:47 UTC ---
Not working on it.

[Bug middle-end/54201] XMM constant duplicated

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #7 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-14 13:20:26 UTC ---
The following works.

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c        (revision 190381)
+++ gcc/varasm.c        (working copy)
@@ -3482,8 +3482,9 @@ force_const_mem (enum machine_mode mode,
 {
   struct constant_descriptor_rtx *desc, tmp;
   struct rtx_constant_pool *pool;
+  enum machine_mode orig_mode = mode;
   char label[256];
-  rtx def, symbol;
+  rtx def, symbol, res;
   hashval_t hash;
   unsigned int align;
   void **slot;
@@ -3500,6 +3501,18 @@ force_const_mem (enum machine_mode mode,
          ? shared_constant_pool
          : crtl->varasm.pool);

+  /* Canonicalize CONST_VECTORs to the mode with the least number of
+     elements assuming that alignment requirements are not worse
+     for the original mode.  */
+  if (GET_CODE (x) == CONST_VECTOR)
+    {
+      while (GET_MODE_SIZE (mode)
+            == GET_MODE_SIZE (GET_MODE_WIDER_MODE (mode)))
+       mode = GET_MODE_WIDER_MODE (mode);
+      x = simplify_subreg (mode, x, orig_mode, 0);
+      gcc_assert (x != NULL_RTX);
+    }
+
   /* Lookup the value in the hashtable.  */
   tmp.constant = x;
   tmp.mode = mode;
@@ -3509,7 +3522,11 @@ force_const_mem (enum machine_mode mode,

   /* If the constant was already present, return its memory.  */
   if (desc)
-    return copy_rtx (desc->mem);
+    {
+      res = copy_rtx (desc->mem);
+      PUT_MODE (res, orig_mode);
+      return res;
+    }

   /* Otherwise, create a new descriptor.  */
   desc = ggc_alloc_constant_descriptor_rtx ();
@@ -3573,7 +3590,9 @@ force_const_mem (enum machine_mode mode,
   if (GET_CODE (x) == LABEL_REF)
     LABEL_PRESERVE_P (XEXP (x, 0)) = 1;

-  return copy_rtx (def);
+  res = copy_rtx (def);
+  PUT_MODE (res, orig_mode);
+  return res;
 }


but we still create two references to the constant pool:

test:
.LFB517:
        .cfi_startproc
        pand    .LC0(%rip), %xmm0
        pcmpeqb .LC0(%rip), %xmm0
        ret

Mine again.

[Bug middle-end/54201] XMM constant duplicated

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-14 14:37:42 UTC ---
Does not help the 2nd testcase btw, because we do not CSE the loads:

        movdqa  .LC0, %xmm3
        movdqa  .LC0, %xmm2
        pand    %xmm3, %xmm0
        pcmpeqb %xmm2, %xmm0
        pand    %xmm0, %xmm1
        pand    %xmm3, %xmm1
        movdqa  %xmm1, %xmm0
        pcmpeqb %xmm2, %xmm0
        ret

so it's only half of the issue.

[Bug middle-end/54201] XMM constant duplicated

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54201

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
         AssignedTo|rguenth at gcc dot gnu.org  |unassigned at gcc dot
                   |                            |gnu.org

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-08-15 09:02:58 UTC ---
I'm no expert on RTL CSE.

[Bug middle-end/54201] XMM constant duplicated

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:0106300f6c3f7bae5eb1c46dbd45aa07c94e1b15

commit r11-2944-g0106300f6c3f7bae5eb1c46dbd45aa07c94e1b15
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Aug 31 10:27:00 2020 +0200

    varasm: Optimize memory broadcast for constant vector under AVX512
[PR54201]

    I meant something like the following, which on e.g. a dumb:

    typedef float V __attribute__((vector_size (4 * sizeof (float))));

    void
    foo (V *p, float *q)
    {
      p[0] += (V) { 1.0f, 2.0f, 3.0f, 4.0f };
      q[0] += 4.0f;
      q[1] -= 3.0f;
      q[17] -= 2.0f;
      q[31] += 1.0f;
    }

    testcase merges all the 4 scalar constant pool entries into the
CONST_VECTOR
    one.

    I'm punting for section anchors and not doing it in the per-function (i.e.
    non-shared) constant pools simply because I don't know them well enough,
    don't know whether backends use the offsets for something etc.
    For section anchors, I guess it would need to be done before (re)computing
the
    offsets and arrange for the desc->mark < 0 entries not to be considered as
    objects in the object block, for non-shared pools, perhaps it would be
    enough to call the new function from output_constant_pool before calling
    recompute_pool_offsets and adjust recompute_pool_offsets to ignore
    desc->mark < 0.

    Here is an adjusted patch that ought to merge even the same sized different
    mode vectors with the same byte representation, etc.
    It won't really help with avoiding the multiple reads of the constant in
the
    same function, but as you found, your patch doesn't help with that either.
    Your patch isn't really incompatible with what the patch below does, though
    I wonder whether a) it wouldn't be better to always canonicalize to an
    integral mode with as few elts as possible even e.g. for floats b) whether
    asserting that it simplify_rtx succeeds is safe, whether it shouldn't just
    canonicalize if the canonicalization works and just do what it previously
    did otherwise.

    The following patch puts all pool entries which can be natively encoded
    into a vector, sorts it by decreasing size, determines minimum size
    of a pool entry and adds hash elts for each (aligned) min_size or wider
    power of two-ish portion of the pool constant in addition to the whole pool
    constant byte representation.

    This is the version that passed bootstrap/regtest on both x86_64-linux and
    i686-linux.  In both bootstraps/regtests together, it saved (from the
    statistics I've gathered) 63104 .rodata bytes (before constant merging),
    in 6814 hits of the data->desc->mark = ~(*slot)->desc->labelno;.

    2020-08-31  Jakub Jelinek  <jakub@redhat.com>

            PR middle-end/54201
            * varasm.c: Include alloc-pool.h.
            (output_constant_pool_contents): Emit desc->mark < 0 entries as
            aliases.
            (struct constant_descriptor_rtx_data): New type.
            (constant_descriptor_rtx_data_cmp): New function.
            (struct const_rtx_data_hasher): New type.
            (const_rtx_data_hasher::hash, const_rtx_data_hasher::equal): New
            methods.
            (optimize_constant_pool): New function.
            (output_shared_constant_pool): Call it if TARGET_SUPPORTS_ALIASES.

[Bug middle-end/54201] XMM constant duplicated

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The .rodata duplication should now be gone with the above commit.

I've tried

--- gcc/varasm.c.jj     2020-08-31 10:26:56.978179325 +0200
+++ gcc/varasm.c        2020-08-31 19:15:34.563274307 +0200
@@ -3818,6 +3818,38 @@ force_const_mem (machine_mode in_mode, r
   if (!is_a <fixed_size_mode> (in_mode, &mode))
     return NULL_RTX;

+  /* Try to canonicalize CONST_VECTORs.  See PR54201.  */
+  if (GET_CODE (x) == CONST_VECTOR)
+    {
+      machine_mode cmode = MIN_MODE_VECTOR_INT;
+      machine_mode altmode = VOIDmode;
+
+      /* Choose a MODE_VECTOR_INT mode with the same size as in_mode
+        and as large as possible element, as long as the element is
+        at most HWI sized (so that CONST_INTs can be used rather
+        than CONST_DOUBLEs or CONST_WIDE_INTs.  */
+      FOR_EACH_MODE_FROM (cmode, cmode)
+       if (known_eq (GET_MODE_SIZE (cmode), GET_MODE_SIZE (in_mode))
+           && known_le (GET_MODE_SIZE (GET_MODE_INNER (cmode)),
+                        HOST_BITS_PER_WIDE_INT)
+           && (altmode == VOIDmode
+               || (GET_MODE_SIZE (GET_MODE_INNER (cmode))
+                   > GET_MODE_SIZE (GET_MODE_INNER (altmode)))))
+         altmode = cmode;
+      if (altmode != VOIDmode && altmode != in_mode)
+       {
+         rtx altx = simplify_subreg (altmode, x, in_mode, 0);
+         if (altx && GET_CODE (altx) == CONST_VECTOR)
+           {
+             rtx altret = force_const_mem (altmode, altx);
+             if (altret)
+               {
+                 PUT_MODE (altret, in_mode);
+                 return altret;
+               }
+           }
+       }
+    }
   /* If we're not allowed to drop X into the constant pool, don't.  */
   if (targetm.cannot_force_const_mem (mode, x))
     return NULL_RTX;

as a variant of Richi's patch, but e.g. on
typedef int V __attribute__((vector_size (16)));
typedef long long int W __attribute__((vector_size (16)));
typedef float X __attribute__((vector_size (16)));

void
foo (V *v, W *w, X *x)
{
  *v += (V) (X) { 2.0f, 2.0f, 2.0f, 2.0f };
  *w += (W) (X) { 2.0f, 2.0f, 2.0f, 2.0f };
  *x += (X) { 2.0f, 2.0f, 2.0f, 2.0f };
}
nothing will really try to share the loads, so the question is if it would gain
us anything.

[Bug middle-end/54201] XMM constant duplicated

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |zingaburga+gcc at hotmail dot com

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 92437 has been marked as a duplicate of this bug. ***

[Bug middle-end/54201] XMM constant duplicated

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

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

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
*** Bug 88922 has been marked as a duplicate of this bug. ***