* [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987)
@ 2012-01-25 8:50 Jakub Jelinek
2012-01-25 9:49 ` Richard Guenther
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2012-01-25 8:50 UTC (permalink / raw)
To: Richard Guenther, Richard Henderson; +Cc: gcc-patches
Hi!
The s390 glibc (_itoa function in particular) is miscompiled because
pcom pass changes:
__asm__("lr %N0,%1
mr %0,%2" : "=&r" __x.__ll : "r" ti_10, "r" s1_12);
__w1_15 = __x.__i.__h;
into:
__x___i___h_lsm0.28_34 = __x.__i.__h;
...
__asm__("lr %N0,%1
mr %0,%2" : "=&r" __x.__ll : "r" ti_10, "r" s1_12);
__w1_15 = __x___i___h_lsm0.28_34;
(where __x is a non-addressable union var of a DImode __ll and a struct of
two SImode __i.__{l,h}. The problem is in get_references_in_stmt:
/* ASM_EXPR and CALL_EXPR may embed arbitrary side effects.
Calls have side-effects, except those to const or pure
functions. */
if ((stmt_code == GIMPLE_CALL
&& !(gimple_call_flags (stmt) & (ECF_CONST | ECF_PURE)))
|| (stmt_code == GIMPLE_ASM
&& gimple_asm_volatile_p (stmt)))
clobbers_memory = true;
is the only place where it handles asm (but the testcase doesn't have
it volatile) and otherwise ignores all the references.
Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
ok for trunk?
BTW, not sure about non-volatile asm that has memory clobber, maybe
the above snippet should be changed into:
|| (stmt_code == GIMPLE_ASM
&& (gimple_asm_volatile_p (stmt)
|| gimple_asm_clobbers_memory_p (stmt)))
2012-01-25 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/51987
* tree-data-ref.c (get_references_in_stmt): Handle references in
non-volatile GIMPLE_ASM.
* gcc.target/i386/pr51987.c: New test.
--- gcc/tree-data-ref.c.jj 2011-11-28 17:58:04.000000000 +0100
+++ gcc/tree-data-ref.c 2012-01-24 22:33:18.995077693 +0100
@@ -1,5 +1,5 @@
/* Data references and dependences detectors.
- Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
+ Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
Free Software Foundation, Inc.
Contributed by Sebastian Pop <pop@cri.ensmp.fr>
@@ -4226,6 +4226,35 @@ get_references_in_stmt (gimple stmt, VEC
}
}
}
+ else if (stmt_code == GIMPLE_ASM)
+ {
+ unsigned i;
+ /* Inputs may perform loads. */
+ for (i = 0; i < gimple_asm_ninputs (stmt); ++i)
+ {
+ op1 = &TREE_VALUE (gimple_asm_input_op (stmt, i));
+ if (DECL_P (*op1)
+ || (REFERENCE_CLASS_P (*op1) && get_base_address (*op1)))
+ {
+ ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
+ ref->pos = op1;
+ ref->is_read = true;
+ }
+ }
+ /* Outputs may perform stores. */
+ for (i = 0; i < gimple_asm_noutputs (stmt); ++i)
+ {
+ op0 = &TREE_VALUE (gimple_asm_output_op (stmt, i));
+ if (DECL_P (*op0)
+ || (REFERENCE_CLASS_P (*op0) && get_base_address (*op0)))
+ {
+ ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
+ ref->pos = op0;
+ ref->is_read = false;
+ }
+ }
+ return clobbers_memory;
+ }
else
return clobbers_memory;
--- gcc/testsuite/gcc.target/i386/pr51987.c.jj 2012-01-24 23:32:44.887896707 +0100
+++ gcc/testsuite/gcc.target/i386/pr51987.c 2012-01-24 23:34:26.289296794 +0100
@@ -0,0 +1,33 @@
+/* PR tree-optimization/51987 */
+/* { dg-do run { target { ! { ia32 } } } } */
+/* { dg-options "-O3" } */
+
+extern void abort (void);
+union U { unsigned long long l; struct { unsigned int l, h; } i; };
+
+__attribute__((noinline, noclone)) void
+foo (char *x, char *y)
+{
+ int i;
+ for (i = 0; i < 64; i++)
+ {
+ union U u;
+ asm ("movl %1, %k0; salq $32, %0" : "=r" (u.l) : "r" (i));
+ x[i] = u.i.h;
+ union U v;
+ asm ("movl %1, %k0; salq $32, %0" : "=r" (v.l) : "r" (i));
+ y[i] = v.i.h;
+ }
+}
+
+int
+main ()
+{
+ char a[64], b[64];
+ int i;
+ foo (a, b);
+ for (i = 0; i < 64; i++)
+ if (a[i] != i || b[i] != i)
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987)
2012-01-25 8:50 [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987) Jakub Jelinek
@ 2012-01-25 9:49 ` Richard Guenther
2012-01-25 11:24 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Richard Guenther @ 2012-01-25 9:49 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches
[-- Attachment #1: Type: TEXT/PLAIN, Size: 5178 bytes --]
On Wed, 25 Jan 2012, Jakub Jelinek wrote:
> Hi!
>
> The s390 glibc (_itoa function in particular) is miscompiled because
> pcom pass changes:
> __asm__("lr %N0,%1
> mr %0,%2" : "=&r" __x.__ll : "r" ti_10, "r" s1_12);
> __w1_15 = __x.__i.__h;
> into:
> __x___i___h_lsm0.28_34 = __x.__i.__h;
> ...
> __asm__("lr %N0,%1
> mr %0,%2" : "=&r" __x.__ll : "r" ti_10, "r" s1_12);
> __w1_15 = __x___i___h_lsm0.28_34;
> (where __x is a non-addressable union var of a DImode __ll and a struct of
> two SImode __i.__{l,h}. The problem is in get_references_in_stmt:
> /* ASM_EXPR and CALL_EXPR may embed arbitrary side effects.
> Calls have side-effects, except those to const or pure
> functions. */
> if ((stmt_code == GIMPLE_CALL
> && !(gimple_call_flags (stmt) & (ECF_CONST | ECF_PURE)))
> || (stmt_code == GIMPLE_ASM
> && gimple_asm_volatile_p (stmt)))
> clobbers_memory = true;
> is the only place where it handles asm (but the testcase doesn't have
> it volatile) and otherwise ignores all the references.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux,
> ok for trunk?
I wonder if it's worth handling asms in any fancy way here, considering
that data-ref analysis happily punts on them completely. Thus, why
not change the above to
|| stmt_code == GIMPLE_ASM)
? That sounds more safe for this stage anyway (does a volatile
asm really clobber memory even if you don't explicitely say so?
At least the operand scanner won't add virtual operands just because of
that, so the check looks bogus anyway).
> BTW, not sure about non-volatile asm that has memory clobber, maybe
> the above snippet should be changed into:
> || (stmt_code == GIMPLE_ASM
> && (gimple_asm_volatile_p (stmt)
> || gimple_asm_clobbers_memory_p (stmt)))
Yeah, and drop the gimple_asm_volatile_p check.
Btw, there are numerous callers of get_references_in_stmt that
do not check its return value ... (and I think it misses a
return value kind that tells the vector of references may be incomplete,
like in the ASM kind right now).
Btw, get_references_in_stmt should probably use walk_stmt_load_store_ops,
so we have a single place where we put knowledge where memory references
can occur.
Thanks,
Richard.
> 2012-01-25 Jakub Jelinek <jakub@redhat.com>
>
> PR tree-optimization/51987
> * tree-data-ref.c (get_references_in_stmt): Handle references in
> non-volatile GIMPLE_ASM.
>
> * gcc.target/i386/pr51987.c: New test.
>
> --- gcc/tree-data-ref.c.jj 2011-11-28 17:58:04.000000000 +0100
> +++ gcc/tree-data-ref.c 2012-01-24 22:33:18.995077693 +0100
> @@ -1,5 +1,5 @@
> /* Data references and dependences detectors.
> - Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
> + Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
> Free Software Foundation, Inc.
> Contributed by Sebastian Pop <pop@cri.ensmp.fr>
>
> @@ -4226,6 +4226,35 @@ get_references_in_stmt (gimple stmt, VEC
> }
> }
> }
> + else if (stmt_code == GIMPLE_ASM)
> + {
> + unsigned i;
> + /* Inputs may perform loads. */
> + for (i = 0; i < gimple_asm_ninputs (stmt); ++i)
> + {
> + op1 = &TREE_VALUE (gimple_asm_input_op (stmt, i));
> + if (DECL_P (*op1)
> + || (REFERENCE_CLASS_P (*op1) && get_base_address (*op1)))
> + {
> + ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
> + ref->pos = op1;
> + ref->is_read = true;
> + }
> + }
> + /* Outputs may perform stores. */
> + for (i = 0; i < gimple_asm_noutputs (stmt); ++i)
> + {
> + op0 = &TREE_VALUE (gimple_asm_output_op (stmt, i));
> + if (DECL_P (*op0)
> + || (REFERENCE_CLASS_P (*op0) && get_base_address (*op0)))
> + {
> + ref = VEC_safe_push (data_ref_loc, heap, *references, NULL);
> + ref->pos = op0;
> + ref->is_read = false;
> + }
> + }
> + return clobbers_memory;
> + }
> else
> return clobbers_memory;
>
> --- gcc/testsuite/gcc.target/i386/pr51987.c.jj 2012-01-24 23:32:44.887896707 +0100
> +++ gcc/testsuite/gcc.target/i386/pr51987.c 2012-01-24 23:34:26.289296794 +0100
> @@ -0,0 +1,33 @@
> +/* PR tree-optimization/51987 */
> +/* { dg-do run { target { ! { ia32 } } } } */
> +/* { dg-options "-O3" } */
> +
> +extern void abort (void);
> +union U { unsigned long long l; struct { unsigned int l, h; } i; };
> +
> +__attribute__((noinline, noclone)) void
> +foo (char *x, char *y)
> +{
> + int i;
> + for (i = 0; i < 64; i++)
> + {
> + union U u;
> + asm ("movl %1, %k0; salq $32, %0" : "=r" (u.l) : "r" (i));
> + x[i] = u.i.h;
> + union U v;
> + asm ("movl %1, %k0; salq $32, %0" : "=r" (v.l) : "r" (i));
> + y[i] = v.i.h;
> + }
> +}
> +
> +int
> +main ()
> +{
> + char a[64], b[64];
> + int i;
> + foo (a, b);
> + for (i = 0; i < 64; i++)
> + if (a[i] != i || b[i] != i)
> + abort ();
> + return 0;
> +}
>
> Jakub
>
>
--
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987)
2012-01-25 9:49 ` Richard Guenther
@ 2012-01-25 11:24 ` Jakub Jelinek
2012-01-25 12:18 ` Richard Guenther
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2012-01-25 11:24 UTC (permalink / raw)
To: Richard Guenther; +Cc: Richard Henderson, gcc-patches
On Wed, Jan 25, 2012 at 10:49:27AM +0100, Richard Guenther wrote:
> I wonder if it's worth handling asms in any fancy way here, considering
> that data-ref analysis happily punts on them completely. Thus, why
> not change the above to
>
> || stmt_code == GIMPLE_ASM)
I think asm ("..." : "+r" (x)); and similar are sufficiently common that
it is worth doing something at least for those.
Thus, I'd be fine with say
(stmt_code == GIMPLE_ASM
&& (gimple_asm_volatile_p (stmt) || gimple_vuse (stmt)))
and don't do anything about the operands.
That would handle the gimple_asm_clobbers_memory_p case, or the case in this
PR, etc.
Would you be ok with that instead? That would be a conservative fix
for this stage.
> ? That sounds more safe for this stage anyway (does a volatile
> asm really clobber memory even if you don't explicitely say so?
> At least the operand scanner won't add virtual operands just because of
> that, so the check looks bogus anyway).
Given that asm volatile should act like an optimization barrier, I think it
is desirable to punt on them even when they don't have memory operands.
> Btw, there are numerous callers of get_references_in_stmt that
> do not check its return value ... (and I think it misses a
> return value kind that tells the vector of references may be incomplete,
> like in the ASM kind right now).
Incomplete should be signalled by returning true I'd say, that means beyond
the listed ones it accesses some others too.
> Btw, get_references_in_stmt should probably use walk_stmt_load_store_ops,
> so we have a single place where we put knowledge where memory references
> can occur.
Probably, but wouldn't that be a 4.8 material?
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987)
2012-01-25 11:24 ` Jakub Jelinek
@ 2012-01-25 12:18 ` Richard Guenther
2012-01-25 15:40 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Richard Guenther @ 2012-01-25 12:18 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Henderson, gcc-patches
On Wed, 25 Jan 2012, Jakub Jelinek wrote:
> On Wed, Jan 25, 2012 at 10:49:27AM +0100, Richard Guenther wrote:
> > I wonder if it's worth handling asms in any fancy way here, considering
> > that data-ref analysis happily punts on them completely. Thus, why
> > not change the above to
> >
> > || stmt_code == GIMPLE_ASM)
>
> I think asm ("..." : "+r" (x)); and similar are sufficiently common that
> it is worth doing something at least for those.
> Thus, I'd be fine with say
> (stmt_code == GIMPLE_ASM
> && (gimple_asm_volatile_p (stmt) || gimple_vuse (stmt)))
> and don't do anything about the operands.
> That would handle the gimple_asm_clobbers_memory_p case, or the case in this
> PR, etc.
> Would you be ok with that instead? That would be a conservative fix
> for this stage.
Yes, that's ok with me. For the rest you can file an enhancement PR.
> > ? That sounds more safe for this stage anyway (does a volatile
> > asm really clobber memory even if you don't explicitely say so?
> > At least the operand scanner won't add virtual operands just because of
> > that, so the check looks bogus anyway).
>
> Given that asm volatile should act like an optimization barrier, I think it
> is desirable to punt on them even when they don't have memory operands.
A volatile asm without explicitely clobbering memory does not act as
an optimization barrier. If it should, then it definitely needs
virtual operands.
Sure, but just like we punt on gimple_has_volatile_ops_p stmts - passes
should explicitely check for gimple_asm_volatile_p or use
gimple_has_side_effects.
> > Btw, there are numerous callers of get_references_in_stmt that
> > do not check its return value ... (and I think it misses a
> > return value kind that tells the vector of references may be incomplete,
> > like in the ASM kind right now).
>
> Incomplete should be signalled by returning true I'd say, that means beyond
> the listed ones it accesses some others too.
>
> > Btw, get_references_in_stmt should probably use walk_stmt_load_store_ops,
> > so we have a single place where we put knowledge where memory references
> > can occur.
>
> Probably, but wouldn't that be a 4.8 material?
Yes.
Thanks,
Richard.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987)
2012-01-25 12:18 ` Richard Guenther
@ 2012-01-25 15:40 ` Jakub Jelinek
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Jelinek @ 2012-01-25 15:40 UTC (permalink / raw)
To: Richard Guenther; +Cc: Richard Henderson, gcc-patches
On Wed, Jan 25, 2012 at 01:18:28PM +0100, Richard Guenther wrote:
> Yes, that's ok with me. For the rest you can file an enhancement PR.
Ok, here is what I've committed, and I'll file the PR momentarily.
2012-01-25 Jakub Jelinek <jakub@redhat.com>
PR tree-optimization/51987
* tree-data-ref.c (get_references_in_stmt): Handle references in
non-volatile GIMPLE_ASM.
* gcc.target/i386/pr51987.c: New test.
--- gcc/tree-data-ref.c.jj 2012-01-25 13:30:51.563590373 +0100
+++ gcc/tree-data-ref.c 2012-01-25 13:34:12.323973880 +0100
@@ -1,5 +1,5 @@
/* Data references and dependences detectors.
- Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
+ Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
Free Software Foundation, Inc.
Contributed by Sebastian Pop <pop@cri.ensmp.fr>
@@ -4185,7 +4185,7 @@ get_references_in_stmt (gimple stmt, VEC
if ((stmt_code == GIMPLE_CALL
&& !(gimple_call_flags (stmt) & (ECF_CONST | ECF_PURE)))
|| (stmt_code == GIMPLE_ASM
- && gimple_asm_volatile_p (stmt)))
+ && (gimple_asm_volatile_p (stmt) || gimple_vuse (stmt))))
clobbers_memory = true;
if (!gimple_vuse (stmt))
--- gcc/testsuite/gcc.target/i386/pr51987.c.jj 2012-01-24 23:32:44.887896707 +0100
+++ gcc/testsuite/gcc.target/i386/pr51987.c 2012-01-24 23:34:26.289296794 +0100
@@ -0,0 +1,33 @@
+/* PR tree-optimization/51987 */
+/* { dg-do run { target { ! { ia32 } } } } */
+/* { dg-options "-O3" } */
+
+extern void abort (void);
+union U { unsigned long long l; struct { unsigned int l, h; } i; };
+
+__attribute__((noinline, noclone)) void
+foo (char *x, char *y)
+{
+ int i;
+ for (i = 0; i < 64; i++)
+ {
+ union U u;
+ asm ("movl %1, %k0; salq $32, %0" : "=r" (u.l) : "r" (i));
+ x[i] = u.i.h;
+ union U v;
+ asm ("movl %1, %k0; salq $32, %0" : "=r" (v.l) : "r" (i));
+ y[i] = v.i.h;
+ }
+}
+
+int
+main ()
+{
+ char a[64], b[64];
+ int i;
+ foo (a, b);
+ for (i = 0; i < 64; i++)
+ if (a[i] != i || b[i] != i)
+ abort ();
+ return 0;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-01-25 15:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-25 8:50 [PATCH] Handle non-volatile GIMPLE_ASM in get_references_in_stmt (PR tree-optimization/51987) Jakub Jelinek
2012-01-25 9:49 ` Richard Guenther
2012-01-25 11:24 ` Jakub Jelinek
2012-01-25 12:18 ` Richard Guenther
2012-01-25 15:40 ` Jakub Jelinek
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).