[Bug middle-end/30778] New: [4.3 Regression] invalid code generation for memset() with -mtune=k8

[Bug middle-end/30778] New: [4.3 Regression] invalid code generation for memset() with -mtune=k8

[belyshev at depni dot sinp dot msu dot ru]

Reduced from combine.c, fails if compiled with "-O1 -mtune=k8", doesn't fail
with "-O1 -mtune=generic".
possibly introduced between r119211 and r119769 (not sure about this).

// -------------------8<-------------------------------------------------------
extern void *memset (void *, int, unsigned long);
extern void abort (void);

struct reg_stat {
  void *last_death;
  void *last_set;
  void *last_set_value;
  int   last_set_label;
  char  last_set_sign_bit_copies;
  int   last_set_mode : 8;
  char  last_set_invalid;
  char sign_bit_copies;
  long nonzero_bits;
};

static struct reg_stat *reg_stat;

void __attribute__((noinline))
init_reg_last (void)
{
  memset (reg_stat, 0, __builtin_offsetof (struct reg_stat, sign_bit_copies));
}

int main (void)
{
  struct reg_stat r;

  reg_stat = &r;
  r.nonzero_bits = -1;
  init_reg_last ();
  if (r.nonzero_bits != -1)
    abort ();
  return 0;
}
// ------------------->8-------------------------------------------------------


-- 
           Summary: [4.3 Regression] invalid code generation for memset()
                    with -mtune=k8
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: belyshev at depni dot sinp dot msu dot ru
GCC target triplet: x86_64-unknown-linux-gnu


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu dot
                   |                            |org
          Component|middle-end                  |target
   Target Milestone|---                         |4.3.0


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[rguenth at gcc dot gnu dot org]

------- Comment #1 from rguenth at gcc dot gnu dot org  2007-02-13 09:43 -------
Confirmed.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2007-02-13 09:43:50
               date|                            |


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[pinskia at gcc dot gnu dot org]

-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |critical


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[ubizjak at gmail dot com]

------- Comment #2 from ubizjak at gmail dot com  2007-02-24 08:53 -------
The problem here is in ix86_expand_set_or_movmem_via_loop().

In mtune=k8 case, we choose unrolled_loop as the algorithm, where main loop is
expanded as

      expand_set_or_movmem_via_loop (dst, NULL, destreg, NULL, promoted_val,
                                     count_exp, Pmode, 4, expected_size);

However, the value of "expected_size" equals 31 in the above testcase! This is
smaller that the smallest chunk size (SIZEOF (Pmode) * 4), requested by caller. 

For some reason, expand_set_or_movmem_via_loop() doesn't detect this anomaly
and generates loop for requested chunk size.

The fix is obvious and straightforward. Bail out of
expand_set_or_movmem_via_loop() early if expected_size < piece_size.

I'll fix this in the evening (CET) if Jan won't beat me to it ;)


, where some kind of guard is missing before entering "Main loop" section.


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ubizjak at gmail dot com


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[ubizjak at gmail dot com]

------- Comment #3 from ubizjak at gmail dot com  2007-02-24 10:59 -------
I'm currently testing this patch:

2007-02-24  Uros Bizjak  <ubizjak@gmail.com>

        * config/i386/i386.md (expand_set_or_movmem_via_loop): Return if
        GET_MODE_SIZE (mode) * unroll is less than expected_size.

testsuite/ChangeLog:

2007-02-24  Uros Bizjak  <ubizjak@gmail.com>

        * gcc.target/i386/pr30778.c: New test.

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c  (revision 122286)
+++ config/i386/i386.c  (working copy)
@@ -13315,13 +13315,19 @@
 {
   rtx out_label, top_label, iter, tmp;
   enum machine_mode iter_mode;
-  rtx piece_size = GEN_INT (GET_MODE_SIZE (mode) * unroll);
-  rtx piece_size_mask = GEN_INT (~((GET_MODE_SIZE (mode) * unroll) - 1));
+  HOST_WIDE_INT min_size = GET_MODE_SIZE (mode) * unroll;
+  rtx piece_size = GEN_INT (min_size);
+  rtx piece_size_mask = GEN_INT (~min_size - 1);
   rtx size;
   rtx x_addr;
   rtx y_addr;
   int i;

+  /* Bail out if expected size is less than minimum size
+     that can be emitted.  */
+  if (expected_size < min_size)
+    return;
+
   iter_mode = GET_MODE (count);
   if (iter_mode == VOIDmode)
     iter_mode = word_mode;

There is also an optimization opportunity. When compiling testcase to 32bit
code (with -m32 -O2 -mtune=k8), following is generated:

init_reg_last:
        pushl   %ebp
        movl    reg_stat, %edx
        xorl    %eax, %eax
        movl    %esp, %ebp
.L2:
        movl    $0, (%edx,%eax)
        movl    $0, 4(%edx,%eax)
        movl    $0, 8(%edx,%eax)
        movl    $0, 12(%edx,%eax)
        addl    $16, %eax
        cmpl    $16, %eax    <<< not needed
        jb      .L2          <<< not needed
        addl    %eax, %edx
        movw    $0, (%edx)
        movb    $0, 2(%edx)
        leave
        ret

We don't need to create loop in this case, as this loop will be executed only
once.


-- 


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[ubizjak at gmail dot com]

-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |ubizjak at gmail dot com
                   |dot org                     |
             Status|NEW                         |ASSIGNED
   Last reconfirmed|2007-02-13 09:43:50         |2007-02-24 22:55:36
               date|                            |


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[hubicka at gcc dot gnu dot org]

------- Comment #4 from hubicka at gcc dot gnu dot org  2007-02-24 23:09 -------
Hi,
testing for expected_size is wrong here - with profile feedback, expected_size
is average size of the block and thus can be smaller than actual size of the
block being copied.
In general we should in this case end up using move_by_pieces so we won't
produce the loop anyway, but still there should be loop header test emitted, I
am just looking on why it is skipped.

Honza


-- 


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[ubizjak at gmail dot com]

------- Comment #5 from ubizjak at gmail dot com  2007-02-24 23:24 -------
(In reply to comment #4)
> Hi,
> testing for expected_size is wrong here - with profile feedback, expected_size
> is average size of the block and thus can be smaller than actual size of the
> block being copied.
> In general we should in this case end up using move_by_pieces so we won't
> produce the loop anyway, but still there should be loop header test emitted, I
> am just looking on why it is skipped.

Please note, that the proposed patch in gcc-patches ML is a bit different and
also removes loops when number of iterations is 1.


-- 


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[hubicka at gcc dot gnu dot org]

------- Comment #6 from hubicka at gcc dot gnu dot org  2007-02-24 23:47 -------
Hi,
this is version I ended up with.  In general I don't think that code should
handle with such a small constant sizes, we probably should bump move_by_pieces
constants since it can do better job using proper moves not triggering partial
memory stalls.

This patch however fixes one extra pasto and makes the prologue test
unconditional - I was a bit overagressive minimizing amount of RTL produced
that only leads to bugs in side corners.

Thanks a lot for looking into it - I should've dealt with this bug a lot
sooner, but I was very busy last week...
Honza

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c  (revision 122292)
+++ config/i386/i386.c  (working copy)
@@ -13522,7 +13522,7 @@ expand_movmem_epilogue (rtx destmem, rtx
       HOST_WIDE_INT countval = INTVAL (count);
       int offset = 0;

-      if ((countval & 0x16) && max_size > 16)
+      if ((countval & 0x10) && max_size > 16)
        {
          if (TARGET_64BIT)
            {
@@ -13539,8 +13539,8 @@ expand_movmem_epilogue (rtx destmem, rtx
            emit_strmov (destmem, srcmem, destptr, srcptr, DImode, offset);
          else
            {
-             emit_strmov (destmem, srcmem, destptr, srcptr, DImode, offset);
-             emit_strmov (destmem, srcmem, destptr, srcptr, DImode, offset +
4);
+             emit_strmov (destmem, srcmem, destptr, srcptr, SImode, offset);
+             emit_strmov (destmem, srcmem, destptr, srcptr, SImode, offset +
4);
            }
          offset += 8;
        }
@@ -13675,7 +13675,7 @@ expand_setmem_epilogue (rtx destmem, rtx
       HOST_WIDE_INT countval = INTVAL (count);
       int offset = 0;

-      if ((countval & 0x16) && max_size > 16)
+      if ((countval & 0x10) && max_size > 16)
        {
          if (TARGET_64BIT)
            {
@@ -14118,19 +14118,22 @@ ix86_expand_movmem (rtx dst, rtx src, rt
   gcc_assert (desired_align >= 1 && align >= 1);

   /* Ensure that alignment prologue won't copy past end of block.  */
-  if ((size_needed > 1 || (desired_align > 1 && desired_align > align))
-      && !count)
+  if (size_needed > 1 || (desired_align > 1 && desired_align > align))
     {
-      epilogue_size_needed = MAX (size_needed - 1, desired_align - align);
+      enum machine_mode mode = GET_MODE (count_exp);

+      if (mode == VOIDmode)
+       mode = DImode; 
+
+      epilogue_size_needed = MAX (size_needed - 1, desired_align - align);
       /* Epilogue always copies COUNT_EXP & EPILOGUE_SIZE_NEEDED bytes.
         Make sure it is power of 2.  */
       epilogue_size_needed = smallest_pow2_greater_than
(epilogue_size_needed);

       label = gen_label_rtx ();
-      emit_cmp_and_jump_insns (count_exp,
+      emit_cmp_and_jump_insns (force_reg (mode, count_exp),
                               GEN_INT (epilogue_size_needed),
-                              LTU, 0, GET_MODE (count_exp), 1, label);
+                              LTU, 0, mode, 1, label);
       if (expected_size == -1 || expected_size < epilogue_size_needed)
        predict_jump (REG_BR_PROB_BASE * 60 / 100);
       else
@@ -14430,11 +14433,14 @@ ix86_expand_setmem (rtx dst, rtx count_e
     promoted_val = promote_duplicated_reg_to_size (val_exp, size_needed,
                                                   desired_align, align);
   /* Ensure that alignment prologue won't copy past end of block.  */
-  if ((size_needed > 1 || (desired_align > 1 && desired_align > align))
-      && !count)
+  if (size_needed > 1 || (desired_align > 1 && desired_align > align))
     {
-      epilogue_size_needed = MAX (size_needed - 1, desired_align - align);
+      enum machine_mode mode = GET_MODE (count_exp);
+
+      if (mode == VOIDmode)
+       mode = DImode; 

+      epilogue_size_needed = MAX (size_needed - 1, desired_align - align);
       /* Epilogue always copies COUNT_EXP & EPILOGUE_SIZE_NEEDED bytes.
         Make sure it is power of 2.  */
       epilogue_size_needed = smallest_pow2_greater_than
(epilogue_size_needed);
@@ -14446,9 +14452,9 @@ ix86_expand_setmem (rtx dst, rtx count_e
       if (epilogue_size_needed > 2 && !promoted_val)
         force_loopy_epilogue = true;
       label = gen_label_rtx ();
-      emit_cmp_and_jump_insns (count_exp,
+      emit_cmp_and_jump_insns (force_reg (mode, count_exp),
                               GEN_INT (epilogue_size_needed),
-                              LTU, 0, GET_MODE (count_exp), 1, label);
+                              LTU, 0, mode, 1, label);
       if (expected_size == -1 || expected_size <= epilogue_size_needed)
        predict_jump (REG_BR_PROB_BASE * 60 / 100);
       else


-- 


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[ubizjak at gmail dot com]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]



------- Comment #7 from ubizjak at gmail dot com  2007-02-25 07:54 -------
(In reply to comment #6)

> This patch however fixes one extra pasto and makes the prologue test
> unconditional - I was a bit overagressive minimizing amount of RTL produced
> that only leads to bugs in side corners.

This patch ICEs for "-O2 -march=nocona -m32" on the testcase above:

pr30778.c: In function ‘init_reg_last’:
pr30778.c:21: internal compiler error: in predict_jump, at
config/i386/i386.c:13246

BTW: For -march=k8, we now generate:

        movq    reg_stat(%rip), %rax
        movq    $0, (%rax)
        addq    $8, %rax
        movq    $0, (%rax)
        addq    $8, %rax
        movq    $0, (%rax)
        addq    $8, %rax
        movl    $0, (%rax)
        addq    $4, %rax
        movw    $0, (%rax)
        movb    $0, 2(%rax)
        ret

and previously:

        movq    reg_stat(%rip), %rax
        movq    $0, (%rax)
        movq    $0, 8(%rax)
        movq    $0, 16(%rax)
        movl    $0, 24(%rax)
        movw    $0, 28(%rax)
        movb    $0, 30(%rax)
        ret

(I don't know if this is intentionally, just want to point it out.)


-- 

ubizjak at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|ubizjak at gmail dot com    |unassigned at gcc dot gnu
                   |                            |dot org
             Status|ASSIGNED                    |NEW


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[hubicka at gcc dot gnu dot org]

------- Comment #8 from hubicka at gcc dot gnu dot org  2007-02-25 17:37 -------
Subject: Bug 30778

Author: hubicka
Date: Sun Feb 25 17:37:37 2007
New Revision: 122313

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=122313
Log:
        PR target/30778
        * i386.c (counter_mode): New function.
        (expand_set_or_movmem_via_loop): Use it.
        (expand_movmem_epilogue): Likewise; fix pasto.
        (ix86_expand_movmem): Do emit guard even for constant counts.
        (ix86_expand_setmem): Likewise.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr30778.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/testsuite/ChangeLog


-- 


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

[Bug target/30778] [4.3 Regression] invalid code generation for memset() with -mtune=k8

[ubizjak at gmail dot com]

------- Comment #9 from ubizjak at gmail dot com  2007-02-26 07:04 -------
Fixed in SVN.


-- 

ubizjak at gmail dot com changed:

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


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