* PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git
@ 2013-12-03 1:07 H.J. Lu
2013-12-03 8:11 ` Uros Bizjak
0 siblings, 1 reply; 6+ messages in thread
From: H.J. Lu @ 2013-12-03 1:07 UTC (permalink / raw)
To: gcc-patches; +Cc: Uros Bizjak
Hi,
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?
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.
gcc/testsuite/
2013-12-03 H.J. Lu <hongjiu.lu@intel.com>
PR target/59363
* gcc.target/i386/pr59363.c: New file.
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index aa221df..d395a99 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;
}
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);
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git
2013-12-03 1:07 PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git H.J. Lu
@ 2013-12-03 8:11 ` Uros Bizjak
2013-12-03 8:45 ` Michael Zolotukhin
0 siblings, 1 reply; 6+ messages in thread
From: Uros Bizjak @ 2013-12-03 8:11 UTC (permalink / raw)
To: H.J. Lu; +Cc: gcc-patches, Michael Zolotukhin, Jan Hubicka
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
Thanks,
Uros.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git
2013-12-03 8:11 ` Uros Bizjak
@ 2013-12-03 8:45 ` Michael Zolotukhin
2013-12-03 12:32 ` H.J. Lu
0 siblings, 1 reply; 6+ messages in thread
From: Michael Zolotukhin @ 2013-12-03 8:45 UTC (permalink / raw)
To: Uros Bizjak; +Cc: H.J. Lu, gcc-patches, Jan Hubicka
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.
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
>
> Thanks,
> Uros.
--
---
Best regards,
Michael V. Zolotukhin,
Software Engineer
Intel Corporation.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git
2013-12-03 8:45 ` Michael Zolotukhin
@ 2013-12-03 12:32 ` H.J. Lu
2013-12-03 12:41 ` Jan Hubicka
0 siblings, 1 reply; 6+ messages in thread
From: H.J. Lu @ 2013-12-03 12:32 UTC (permalink / raw)
To: Michael Zolotukhin; +Cc: Uros Bizjak, gcc-patches, Jan Hubicka
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.
--
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.
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);
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git
2013-12-03 12:32 ` H.J. Lu
@ 2013-12-03 12:41 ` Jan Hubicka
2013-12-03 12:57 ` H.J. Lu
0 siblings, 1 reply; 6+ messages in thread
From: Jan Hubicka @ 2013-12-03 12:41 UTC (permalink / raw)
To: H.J. Lu; +Cc: Michael Zolotukhin, Uros Bizjak, gcc-patches, Jan Hubicka
> 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);
> +}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git
2013-12-03 12:41 ` Jan Hubicka
@ 2013-12-03 12:57 ` H.J. Lu
0 siblings, 0 replies; 6+ messages in thread
From: H.J. Lu @ 2013-12-03 12:57 UTC (permalink / raw)
To: Jan Hubicka; +Cc: Michael Zolotukhin, Uros Bizjak, gcc-patches
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.
--
H.J.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-03 12:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-03 1:07 PATCH: PR target/59363: [4.9 Regression] r203886 miscompiles git H.J. Lu
2013-12-03 8:11 ` Uros Bizjak
2013-12-03 8:45 ` Michael Zolotukhin
2013-12-03 12:32 ` H.J. Lu
2013-12-03 12:41 ` Jan Hubicka
2013-12-03 12:57 ` H.J. Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).