diff mbox

PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git

Message ID CAMe9rOqkmkvBVFQ-=80HqCPmD21kHXK-vqJMkPhgP+DpTWa2aA@mail.gmail.com
State New
Headers show

Commit Message

H.J. Lu Dec. 3, 2013, 12:32 p.m. UTC 
On Tue, Dec 3, 2013 at 12:45 AM, Michael Zolotukhin
<michael.v.zolotukhin@gmail.com> wrote:
> Hi Uros, HJ,
>
> I checked expand_movmem_epilogue - it seemingly doesn't have such a
> problem, so the patch is ok.
>
> We might want to add similar adjust_automodify_address_nv call to here as well:
>     if (TARGET_64BIT)
>         {
>           dest = change_address (destmem, DImode, destptr);
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>         }
>       else
>         {
>           dest = change_address (destmem, SImode, destptr);
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>           emit_insn (gen_strset (destptr, dest, value));
>         }
> (code snippet from previous HJ's comment in bugzilla).
> I think it's needed here, but I didn't manage to exploit the bug in
> this code. Maybe Uros or Jan can comment whether it's needed in such
> code or not.

It is almost impossible to reach those codes with current
memset/memcpy strategy.  expand_setmem_epilogue is
called either with a constant count or max_size > 32.
None of them will trigger those codes.  But we should
fix them with proper address.  Otherwise, we will run
into the similar problem if they are used one day in
the future.

> Thanks,
> Michael
>
>
> On 3 December 2013 12:11, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Tue, Dec 3, 2013 at 2:05 AM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>
>>> emit_memset fails to adjust destination address after gen_strset, which
>>> leads to the wrong address in aliasing info.  This patch fixes it.
>>> Tested on Linux/x86-64.  OK to install?
>>>
>>> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>         PR target/59363
>>>         * config/i386/i386.c (emit_memset): Adjust destination address
>>>         after gen_strset.
>>>
>>> gcc/testsuite/
>>>
>>> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
>>>
>>>         PR target/59363
>>>         * gcc.target/i386/pr59363.c: New file.
>>
>> OK, but according to [1], there are other places where similar issues
>> should be fixed. I propose to wait for Michael's analysis and eventual
>> patch, and fix them all together.
>>
>> [1] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59363#c21
>>

Here is the updated patch.  Tested on Linux/x86-64.  It
fixed git.  OK to install?

Thanks.

Comments

Jan Hubicka Dec. 3, 2013, 12:41 p.m. UTC | #1 
> Here is the updated patch.  Tested on Linux/x86-64.  It
> fixed git.  OK to install?
> 
> Thanks.
> 
> -- 
> H.J.
> ---
> gcc/
> 
> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
> 
>     PR target/59363
>     * config/i386/i386.c (emit_memset): Adjust destination address
>     after gen_strset.
>     (expand_setmem_epilogue): Likewise.
> 
> gcc/testsuite/
> 
> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
> 
>     PR target/59363
>     * gcc.target/i386/pr59363.c: New file.

Yes, this seems fine to me.  As discussed previously, we probably want to make
strmov patterns use to match strset (I will need to re-check codegen on targets
that does single memops) and then we will need similar update of aliasing
there, too.
Currently I assume we are fine becaue we use it only in expand_movmem epilogue
and on the way there we already cleared the alias offset on all code paths?

Thanks for looking into it.
Honza
> 
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index b11363be..d048511 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -22806,6 +22806,8 @@ emit_memset (rtx destmem, rtx destptr, rtx promoted_val,
>        if (piece_size <= GET_MODE_SIZE (word_mode))
>      {
>        emit_insn (gen_strset (destptr, dst, promoted_val));
> +      dst = adjust_automodify_address_nv (dst, move_mode, destptr,
> +                          piece_size);
>        continue;
>      }
> 
> @@ -22875,14 +22877,18 @@ expand_setmem_epilogue (rtx destmem, rtx
> destptr, rtx value, rtx vec_value,
>      {
>        dest = change_address (destmem, DImode, destptr);
>        emit_insn (gen_strset (destptr, dest, value));
> +      dest = adjust_automodify_address_nv (dest, DImode, destptr, 8);
>        emit_insn (gen_strset (destptr, dest, value));
>      }
>        else
>      {
>        dest = change_address (destmem, SImode, destptr);
>        emit_insn (gen_strset (destptr, dest, value));
> +      dest = adjust_automodify_address_nv (dest, SImode, destptr, 4);
>        emit_insn (gen_strset (destptr, dest, value));
> +      dest = adjust_automodify_address_nv (dest, SImode, destptr, 8);
>        emit_insn (gen_strset (destptr, dest, value));
> +      dest = adjust_automodify_address_nv (dest, SImode, destptr, 12);
>        emit_insn (gen_strset (destptr, dest, value));
>      }
>        emit_label (label);
> @@ -22900,6 +22906,7 @@ expand_setmem_epilogue (rtx destmem, rtx
> destptr, rtx value, rtx vec_value,
>      {
>        dest = change_address (destmem, SImode, destptr);
>        emit_insn (gen_strset (destptr, dest, value));
> +      dest = adjust_automodify_address_nv (dest, SImode, destptr, 4);
>        emit_insn (gen_strset (destptr, dest, value));
>      }
>        emit_label (label);
> diff --git a/gcc/testsuite/gcc.target/i386/pr59363.c
> b/gcc/testsuite/gcc.target/i386/pr59363.c
> new file mode 100644
> index 0000000..a4e1240
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr59363.c
> @@ -0,0 +1,24 @@
> +/* PR target/59363 */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -mtune=amdfam10" } */
> +
> +typedef struct {
> +  int ctxlen;
> +  long interhunkctxlen;
> +  int flags;
> +  long find_func;
> +  void *find_func_priv;
> +  int hunk_func;
> +} xdemitconf_t;
> +
> +__attribute__((noinline))
> +int xdi_diff(xdemitconf_t *xecfg) {
> +  if (xecfg->hunk_func == 0)
> +    __builtin_abort();
> +  return 0;
> +}
> +int main() {
> +  xdemitconf_t xecfg = {0};
> +  xecfg.hunk_func = 1;
> +  return xdi_diff(&xecfg);
> +}
H.J. Lu Dec. 3, 2013, 12:57 p.m. UTC | #2 
On Tue, Dec 3, 2013 at 4:41 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Here is the updated patch.  Tested on Linux/x86-64.  It
>> fixed git.  OK to install?
>>
>> Thanks.
>>
>> --
>> H.J.
>> ---
>> gcc/
>>
>> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
>>
>>     PR target/59363
>>     * config/i386/i386.c (emit_memset): Adjust destination address
>>     after gen_strset.
>>     (expand_setmem_epilogue): Likewise.
>>
>> gcc/testsuite/
>>
>> 2013-12-03   H.J. Lu  <hongjiu.lu@intel.com>
>>
>>     PR target/59363
>>     * gcc.target/i386/pr59363.c: New file.
>
> Yes, this seems fine to me.  As discussed previously, we probably want to make
> strmov patterns use to match strset (I will need to re-check codegen on targets
> that does single memops) and then we will need similar update of aliasing
> there, too.
> Currently I assume we are fine becaue we use it only in expand_movmem epilogue
> and on the way there we already cleared the alias offset on all code paths?
>

I believe it is the case.

I checked it in.

Thanks.
diff mbox

Patch

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index b11363be..d048511 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -22806,6 +22806,8 @@  emit_memset (rtx destmem, rtx destptr, rtx promoted_val,
       if (piece_size <= GET_MODE_SIZE (word_mode))
     {
       emit_insn (gen_strset (destptr, dst, promoted_val));
+      dst = adjust_automodify_address_nv (dst, move_mode, destptr,
+                          piece_size);
       continue;
     }

@@ -22875,14 +22877,18 @@  expand_setmem_epilogue (rtx destmem, rtx
destptr, rtx value, rtx vec_value,
     {
       dest = change_address (destmem, DImode, destptr);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, DImode, destptr, 8);
       emit_insn (gen_strset (destptr, dest, value));
     }
       else
     {
       dest = change_address (destmem, SImode, destptr);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 4);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 8);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 12);
       emit_insn (gen_strset (destptr, dest, value));
     }
       emit_label (label);
@@ -22900,6 +22906,7 @@  expand_setmem_epilogue (rtx destmem, rtx
destptr, rtx value, rtx vec_value,
     {
       dest = change_address (destmem, SImode, destptr);
       emit_insn (gen_strset (destptr, dest, value));
+      dest = adjust_automodify_address_nv (dest, SImode, destptr, 4);
       emit_insn (gen_strset (destptr, dest, value));
     }
       emit_label (label);
diff --git a/gcc/testsuite/gcc.target/i386/pr59363.c
b/gcc/testsuite/gcc.target/i386/pr59363.c
new file mode 100644
index 0000000..a4e1240
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr59363.c
@@ -0,0 +1,24 @@ 
+/* PR target/59363 */
+/* { dg-do run } */
+/* { dg-options "-O2 -mtune=amdfam10" } */
+
+typedef struct {
+  int ctxlen;
+  long interhunkctxlen;
+  int flags;
+  long find_func;
+  void *find_func_priv;
+  int hunk_func;
+} xdemitconf_t;
+
+__attribute__((noinline))
+int xdi_diff(xdemitconf_t *xecfg) {
+  if (xecfg->hunk_func == 0)
+    __builtin_abort();
+  return 0;
+}
+int main() {
+  xdemitconf_t xecfg = {0};
+  xecfg.hunk_func = 1;
+  return xdi_diff(&xecfg);
+}