diff mbox

[PR58805] Add missing check in stmt_local_def for tail-merge

Message ID 52664C2B.60400@mentor.com
State New
Headers show

Commit Message

Tom de Vries Oct. 22, 2013, 9:58 a.m. UTC
Richard,

This patch adds a missing check for gimple_vdef in stmt_local_def for the
tail-merge pass.

Bootstrapped and reg-tested on x86_64.

OK for trunk, gcc-4_8-branch?

Thanks,
- Tom

2013-10-22  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/58805
	* tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check.

	* gcc.dg/pr58805.c: New test.

Comments

Jeff Law Oct. 22, 2013, 6:50 p.m. UTC | #1
On 10/22/13 03:58, Tom de Vries wrote:
> Richard,
>
> This patch adds a missing check for gimple_vdef in stmt_local_def for the
> tail-merge pass.
>
> Bootstrapped and reg-tested on x86_64.
>
> OK for trunk, gcc-4_8-branch?
>
> Thanks,
> - Tom
>
> 2013-10-22  Tom de Vries  <tom@codesourcery.com>
>
> 	PR tree-optimization/58805
> 	* tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check.
>
> 	* gcc.dg/pr58805.c: New test.
Doesn't this test belong in an architecture specific directory?

Under what conditions can a statement have a VDEF but not be considered 
as having a side effect by gimple_has_side_effects?

It almost seems to me that gimple_has_side_effects may need updating.

jeff
Richard Biener Oct. 23, 2013, 8:16 a.m. UTC | #2
On Tue, 22 Oct 2013, Jeff Law wrote:

> On 10/22/13 03:58, Tom de Vries wrote:
> > Richard,
> > 
> > This patch adds a missing check for gimple_vdef in stmt_local_def for the
> > tail-merge pass.
> > 
> > Bootstrapped and reg-tested on x86_64.
> > 
> > OK for trunk, gcc-4_8-branch?
> > 
> > Thanks,
> > - Tom
> > 
> > 2013-10-22  Tom de Vries  <tom@codesourcery.com>
> > 
> > 	PR tree-optimization/58805
> > 	* tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check.
> > 
> > 	* gcc.dg/pr58805.c: New test.
> Doesn't this test belong in an architecture specific directory?
> 
> Under what conditions can a statement have a VDEF but not be considered as
> having a side effect by gimple_has_side_effects?
> 
> It almost seems to me that gimple_has_side_effects may need updating.

You seem to misunderstand "side-effect", for example

 *p = 1;

has !gimple_has_side_effects but it has a VDEF.  Likewise

 *p = const_call_returing_aggregate ();

has !gimple_has_side_effects but it has a VDEF.  side-effect is
an effect that is not explicitely represented in the gimple stmt
you look at.

Richard.
Richard Biener Oct. 23, 2013, 8:16 a.m. UTC | #3
On Tue, 22 Oct 2013, Tom de Vries wrote:

> Richard,
> 
> This patch adds a missing check for gimple_vdef in stmt_local_def for the
> tail-merge pass.
> 
> Bootstrapped and reg-tested on x86_64.
> 
> OK for trunk, gcc-4_8-branch?

Ok.

Thanks,
Richard.
Tom de Vries Oct. 23, 2013, 4:02 p.m. UTC | #4
On 22/10/13 20:50, Jeff Law wrote:
> On 10/22/13 03:58, Tom de Vries wrote:
>> Richard,
>>
>> This patch adds a missing check for gimple_vdef in stmt_local_def for the
>> tail-merge pass.
>>
>> Bootstrapped and reg-tested on x86_64.
>>
>> OK for trunk, gcc-4_8-branch?
>>
>> Thanks,
>> - Tom
>>
>> 2013-10-22  Tom de Vries  <tom@codesourcery.com>
>>
>> 	PR tree-optimization/58805
>> 	* tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check.
>>
>> 	* gcc.dg/pr58805.c: New test.
> Doesn't this test belong in an architecture specific directory?
> 

Jeff,

The test-case has i386 assembly inside the asm string, but since the test-case
only compiles, the assembly string is never used. I've made the string empty to
make that clear.

AFAIU the only requirement for this test-case is that the constraint matches the
operand. I'm not sure whether 'unsigned long' always matches 'r'. I've changed
this into 'void *' and 'p', which I think should always be true.

Committed as below.

Thanks,
- Tom

> jeff
> 

+++ b/gcc/testsuite/gcc.dg/pr58805.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -ftree-tail-merge -fdump-tree-pre" } */
+
+/* Type that matches the 'p' constraint.  */
+#define TYPE void *
+
+static inline
+void bar (TYPE *r)
+{
+  TYPE t;
+  __asm__ ("" : "=&p" (t), "=p" (*r));
+}
+
+void
+foo (int n, TYPE *x, TYPE *y)
+{
+  if (n == 0)
+    bar (x);
+  else
+    bar (y);
+}
+
+/* { dg-final { scan-tree-dump-times "__asm__" 2 "pre"} } */
+/* { dg-final { cleanup-tree-dump "pre" } } */
Jeff Law Oct. 23, 2013, 7:55 p.m. UTC | #5
On 10/23/13 10:02, Tom de Vries wrote:
> On 22/10/13 20:50, Jeff Law wrote:
>> On 10/22/13 03:58, Tom de Vries wrote:
>>> Richard,
>>>
>>> This patch adds a missing check for gimple_vdef in stmt_local_def for the
>>> tail-merge pass.
>>>
>>> Bootstrapped and reg-tested on x86_64.
>>>
>>> OK for trunk, gcc-4_8-branch?
>>>
>>> Thanks,
>>> - Tom
>>>
>>> 2013-10-22  Tom de Vries  <tom@codesourcery.com>
>>>
>>> 	PR tree-optimization/58805
>>> 	* tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check.
>>>
>>> 	* gcc.dg/pr58805.c: New test.
>> Doesn't this test belong in an architecture specific directory?
>>
>
> Jeff,
>
> The test-case has i386 assembly inside the asm string, but since the test-case
> only compiles, the assembly string is never used. I've made the string empty to
> make that clear.
Thanks.




>
> AFAIU the only requirement for this test-case is that the constraint matches the
> operand. I'm not sure whether 'unsigned long' always matches 'r'. I've changed
> this into 'void *' and 'p', which I think should always be true.
>
> Committed as below.
Excellent.  Thanks again.

jeff
Jeff Law Oct. 23, 2013, 7:57 p.m. UTC | #6
On 10/23/13 02:16, Richard Biener wrote:
> On Tue, 22 Oct 2013, Jeff Law wrote:
>
>> On 10/22/13 03:58, Tom de Vries wrote:
>>> Richard,
>>>
>>> This patch adds a missing check for gimple_vdef in stmt_local_def for the
>>> tail-merge pass.
>>>
>>> Bootstrapped and reg-tested on x86_64.
>>>
>>> OK for trunk, gcc-4_8-branch?
>>>
>>> Thanks,
>>> - Tom
>>>
>>> 2013-10-22  Tom de Vries  <tom@codesourcery.com>
>>>
>>> 	PR tree-optimization/58805
>>> 	* tree-ssa-tail-merge.c (stmt_local_def): Add gimple_vdef check.
>>>
>>> 	* gcc.dg/pr58805.c: New test.
>> Doesn't this test belong in an architecture specific directory?
>>
>> Under what conditions can a statement have a VDEF but not be considered as
>> having a side effect by gimple_has_side_effects?
>>
>> It almost seems to me that gimple_has_side_effects may need updating.
>
> You seem to misunderstand "side-effect", for example
>
>   *p = 1;
>
> has !gimple_has_side_effects but it has a VDEF.  Likewise
>
>   *p = const_call_returing_aggregate ();
>
> has !gimple_has_side_effects but it has a VDEF.  side-effect is
> an effect that is not explicitely represented in the gimple stmt
> you look at.
So side effects as in implicit...  That makes sense.  Thanks for the 
clarification.
jeff
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/pr58805.c b/gcc/testsuite/gcc.dg/pr58805.c
new file mode 100644
index 0000000..6e6eba5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr58805.c
@@ -0,0 +1,23 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -ftree-tail-merge -fdump-tree-pre" } */
+
+static inline void bar(unsigned long *r)
+{
+  unsigned long t;
+  __asm__ (
+    "movq $42, %[t]\n\t"
+    "movq %[t], %[r]\n\t"
+    : [t] "=&r" (t), [r] "=r" (*r)
+  );
+}
+
+void foo(int n, unsigned long *x, unsigned long *y)
+{
+  if (n == 0)
+    bar(x);
+  else
+    bar(y);
+}
+
+/* { dg-final { scan-tree-dump-times "__asm__" 2 "pre"} } */
+/* { dg-final { cleanup-tree-dump "pre" } } */
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index 9094935..0090da6 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -300,7 +300,8 @@  stmt_local_def (gimple stmt)
   tree val;
   def_operand_p def_p;
 
-  if (gimple_has_side_effects (stmt))
+  if (gimple_has_side_effects (stmt)
+      || gimple_vdef (stmt) != NULL_TREE)
     return false;
 
   def_p = SINGLE_SSA_DEF_OPERAND (stmt, SSA_OP_DEF);