Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, 16 Mar 2012 11:10:48 +0100
Richard Guenther richard.guent...@gmail.com wrote:
On Fri, Mar 16, 2012 at 1:29 AM, Jonathan Wakely
jwakely@gmail.com wrote:
On 15 March 2012 15:40, Richard Guenther wrote:
On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz
ktiet...@googlemail.com wrote:
Richard,
ping. I think now could be a good time for applying the patch you
have for this issue as we are in stage 1.
It will still regress the two libstdc++ testcases (well, I guess
so at least).
Jonathan - you didn't answer my reply to your question? Would it
be ok to apply this patch with leaving the regressions in-place,
to be investigated by libstdc++ folks?
Sorry, I've either forgotten or missed the reply - but if you think
the problem is in libstdc++ then certainly go ahead and apply it,
I'll investigate the libstdc++ problems (and ask for help if they
defeat me!)
Ok. I'll do so after re-testing the patch.
FYI, here is the patch for the new libstdc++ fails.
-benjamin
2012-03-19 Benjamin Kosnik b...@redhat.com
* include/ext/pb_ds/detail/pat_trie_/
constructors_destructor_fn_imps.hpp: Increment after recursion.
* include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp: Convert
node_type markup from brief.
diff --git a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp
index 8370a2e..c5748ec 100644
--- a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp
+++ b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/constructors_destructor_fn_imps.hpp
@@ -1,6 +1,6 @@
// -*- C++ -*-
-// Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011
+// Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
// Free Software Foundation, Inc.
//
// This file is part of the GNU ISO C++ Library. This library is free
@@ -188,7 +188,11 @@ recursive_copy_node(node_const_pointer p_ncp)
__try
{
while (child_it != p_icp-end())
- a_p_children[child_i++] = recursive_copy_node(*(child_it++));
+ {
+ a_p_children[child_i] = recursive_copy_node(*(child_it));
+ child_i++;
+ child_it++;
+ }
p_ret = s_inode_allocator.allocate(1);
}
__catch(...)
diff --git a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp
index b7eb024..0a763b5 100644
--- a/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp
+++ b/libstdc++-v3/include/ext/pb_ds/detail/pat_trie_/pat_trie_base.hpp
@@ -1,6 +1,6 @@
// -*- C++ -*-
-// Copyright (C) 2005, 2006, 2009, 2011 Free Software Foundation, Inc.
+// Copyright (C) 2005, 2006, 2009, 2011, 2012 Free Software Foundation, Inc.
//
// This file is part of the GNU ISO C++ Library. This library is free
// software; you can redistribute it and/or modify it under the terms
@@ -50,7 +50,11 @@ namespace __gnu_pbds
/// Base type for PATRICIA trees.
struct pat_trie_base
{
- /// Three types of nodes.
+ /**
+ * @brief Three types of nodes.
+ *
+ * i_node is used by _Inode, leaf_node by _Leaf, and head_node by _Head.
+ */
enum node_type
{
i_node,
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, Mar 16, 2012 at 1:29 AM, Jonathan Wakely jwakely@gmail.com wrote: On 15 March 2012 15:40, Richard Guenther wrote: On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz ktiet...@googlemail.com wrote: Richard, ping. I think now could be a good time for applying the patch you have for this issue as we are in stage 1. It will still regress the two libstdc++ testcases (well, I guess so at least). Jonathan - you didn't answer my reply to your question? Would it be ok to apply this patch with leaving the regressions in-place, to be investigated by libstdc++ folks? Sorry, I've either forgotten or missed the reply - but if you think the problem is in libstdc++ then certainly go ahead and apply it, I'll investigate the libstdc++ problems (and ask for help if they defeat me!) Ok. I'll do so after re-testing the patch. Richard.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On 15 March 2012 15:40, Richard Guenther wrote: On Thu, Mar 15, 2012 at 4:22 PM, Kai Tietz ktiet...@googlemail.com wrote: Richard, ping. I think now could be a good time for applying the patch you have for this issue as we are in stage 1. It will still regress the two libstdc++ testcases (well, I guess so at least). Jonathan - you didn't answer my reply to your question? Would it be ok to apply this patch with leaving the regressions in-place, to be investigated by libstdc++ folks? Sorry, I've either forgotten or missed the reply - but if you think the problem is in libstdc++ then certainly go ahead and apply it, I'll investigate the libstdc++ problems (and ask for help if they defeat me!)
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz ktiet...@googlemail.com wrote:
2012/2/9 Richard Guenther richard.guent...@gmail.com:
Works apart from
Running target unix/
FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
Maybe invalid testcases. Who knows ... same fails happen with your patch.
Richard.
Richard.
Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some
use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause
part of this failure.
This might lead here to this failure. I am not sure, if such
constructs having fixed behavior for C++, but it looks to me like
undefined behavior.
A C-testcase for the issue would be:
int *foo (int *p)
{
*p++ = *p;
return p;
}
which produces with patch now:
foo (int * p)
{
int * D.1363;
int D.1364;
int * D.1365;
D.1363 = p;
p = D.1363 + 4;
D.1364 = *p;
*D.1363 = D.1364;
D.1365 = p;
return D.1365;
}
but in old variant we were producing:
foo (int * p)
{
int D.1363;
int * D.1364;
D.1363 = *p;
*p = D.1363;
p = p + 4;
D.1364 = p;
return D.1364;
}
So, maybe the real solution for this issue might be to swap for
assignment gimplification the order for lhs/rhs gimplification
instead.
Well, that would certainly not be suitable for stage4 (though I have a working
patch for that as well). The post-modify gimplification change looks better
as it also fixes the volatile aggregate-return case which would not be fixed
by re-ordering of the gimplification.
libstdc++ folks - can you investigate the testsuite failure?
The patch in question is at
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00435/fix-pr48814
Note that the _Mutable_ForwardIteratorConcept isn't the problem I think,
it's not executed code.
Thanks,
Richard.
Regards,
Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
Kai Tietz ktiet...@googlemail.com writes: Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause part of this failure. I don't think those __constraints functions are ever executed. They are only present to assert the presense of certain operations at compile time. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On 10 February 2012 10:35, Richard Guenther wrote: Works apart from Running target unix/ FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test What does libstdc++.log show for those failures?
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Fri, Feb 10, 2012 at 2:12 PM, Jonathan Wakely jwakely@gmail.com wrote: On 10 February 2012 10:35, Richard Guenther wrote: Works apart from Running target unix/ FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test What does libstdc++.log show for those failures? spawn [open ...]^M ?xml version = 1.0? test sd value = 1328880244 /sd n value = 5000 /n m value = 1 /m tp value = 0.2 /tp ip value = 0.6 /ip ep value = 0.2 /ep cp value = 0.001 /cp mp value = 0.25 /mp /test cntnr name = pat_trie_map desc type value = trie Tag value = pat_trie_tag /Tag Node_Update value = null_node_update /Node_Update /type /desc progress *FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test spawn [open ...]^M ?xml version = 1.0? test sd value = 1328880487 /sd n value = 5000 /n m value = 1 /m tp value = 0.2 /tp ip value = 0.6 /ip ep value = 0.2 /ep cp value = 0.001 /cp mp value = 0.25 /mp /test cntnr name = pat_trie_set desc type value = trie Tag value = pat_trie_tag /Tag Node_Update value = null_node_update /Node_Update /type /desc progress **FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/2/10 Richard Guenther richard.guent...@gmail.com:
On Fri, Feb 10, 2012 at 9:36 AM, Kai Tietz ktiet...@googlemail.com wrote:
2012/2/9 Richard Guenther richard.guent...@gmail.com:
Works apart from
Running target unix/
FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
Maybe invalid testcases. Who knows ... same fails happen with your patch.
Richard.
Richard.
Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some
use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause
part of this failure.
This might lead here to this failure. I am not sure, if such
constructs having fixed behavior for C++, but it looks to me like
undefined behavior.
A C-testcase for the issue would be:
int *foo (int *p)
{
*p++ = *p;
return p;
}
which produces with patch now:
foo (int * p)
{
int * D.1363;
int D.1364;
int * D.1365;
D.1363 = p;
p = D.1363 + 4;
D.1364 = *p;
*D.1363 = D.1364;
D.1365 = p;
return D.1365;
}
but in old variant we were producing:
foo (int * p)
{
int D.1363;
int * D.1364;
D.1363 = *p;
*p = D.1363;
p = p + 4;
D.1364 = p;
return D.1364;
}
So, maybe the real solution for this issue might be to swap for
assignment gimplification the order for lhs/rhs gimplification
instead.
Well, that would certainly not be suitable for stage4 (though I have a working
patch for that as well). The post-modify gimplification change looks better
as it also fixes the volatile aggregate-return case which would not be fixed
by re-ordering of the gimplification.
libstdc++ folks - can you investigate the testsuite failure?
The patch in question is at
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00435/fix-pr48814
Note that the _Mutable_ForwardIteratorConcept isn't the problem I think,
it's not executed code.
Thanks,
Richard.
Hmm, we might need here lhs/rhs flip plus the post-modify. At least
we would avoid by this code differences for common cases that lhs has
post-inc/dec to current behavior?
But you might be right that this patch is not suitable for stage 4.
Regards,
Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/11 Richard Guenther richard.guent...@gmail.com:
count despite being declared volatile and only loaded once in the source
is loaded twice in gimple. If it were a HW register which destroys the
device after the 2nd load without an intervening store you'd wrecked
the device ;)
Richard.
Thanks for explaination. I tried to flip order for lhs/rhs in
gimplify_modify_expr co. Issue here is that for some cases we are
relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
like:
typedef const unsigned char _Jv_Utf8Const;
typedef __SIZE_TYPE__ uaddr;
void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special)
{
union {
_Jv_Utf8Const *signature;
uaddr signature_bits;
};
signature = s;
special = signature_bits 1;
signature_bits -= special;
s = signature;
}
So I modified gimplify_self_mod_expr for post-inc/dec so that we use
following sequence
and add it to pre_p for it:
tmp = lhs;
lvalue = tmp (+/-) rhs
*expr_p = tmp;
As I explained this is the wrong place to fix the PR. The issue is not
about self-modifying expressions but about evaluating call argument
side-effects before side-effects of the lhs.
Richard.
ChangeLog
2012-02-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Move for
postfix-inc/dec the modification in pre and return
temporary with origin value.
2012-02-08 Kai Tietz kti...@redhat.com
* gcc.c-torture/execute/pr48814-1.c: New test.
* gcc.c-torture/execute/pr48814-2.c: New test.
* gcc.dg/tree-ssa/assign-1.c: New test.
* gcc.dg/tree-ssa/assign-2.c: New test.
I did boostrap for all languages (including Ada and Obj-C++) and
regression tests on host x86_64-unknown-linux-gnu. Ok for apply?
Regards,
Kai
Index: gcc/gcc/gimplify.c
===
--- gcc.orig/gcc/gimplify.c
+++ gcc/gcc/gimplify.c
@@ -2197,7 +2197,7 @@ gimplify_self_mod_expr (tree *expr_p, gi
bool want_value)
{
enum tree_code code;
- tree lhs, lvalue, rhs, t1;
+ tree lhs, lvalue, rhs, t1, t2;
gimple_seq post = NULL, *orig_post_p = post_p;
bool postfix;
enum tree_code arith_code;
@@ -2264,20 +2264,23 @@ gimplify_self_mod_expr (tree *expr_p, gi
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
-
- if (postfix)
- {
- gimplify_assign (lvalue, t1, orig_post_p);
- gimplify_seq_add_seq (orig_post_p, post);
- *expr_p = lhs;
- return GS_ALL_DONE;
- }
- else
+ if (!postfix)
{
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
*expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1);
return GS_OK;
}
+
+ /* Assign lhs to temporary variable. */
+ t2 = create_tmp_var (TREE_TYPE (lhs), NULL);
+ gimplify_assign (t2, lhs, pre_p);
+ /* Do increment and assign it to lvalue. */
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), t2, rhs);
+ gimplify_assign (lvalue, t1, pre_p);
+
+ gimplify_seq_add_seq (orig_post_p, post);
+ *expr_p = t2;
+ return GS_ALL_DONE;
}
/* If *EXPR_P has a variable sized type, wrap it in a WITH_SIZE_EXPR. */
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int __attribute__((noinline))
+incr (void)
+{
+ return ++count;
+}
+
+int main()
+{
+ arr[count++] = incr ();
+ if (count != 2 || arr[count] != 3)
+ abort ();
+ return 0;
+}
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int
+incr (void)
+{
+ return ++count;
+}
+
+int main()
+{
+ arr[count++] = incr ();
+ if (count != 2 || arr[count] != 3)
+ abort ();
+ return 0;
+}
Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options -O2 -fdump-tree-optimized } */
+
+volatile int count;
+void bar(int);
+void foo()
+{
+ bar(count++);
+}
+
+/* { dg-final { scan-tree-dump-times count = 1 optimized} } */
+/* { dg-final { cleanup-tree-dump optimized } } */
Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
===
---
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/11 Richard Guenther richard.guent...@gmail.com:
count despite being declared volatile and only loaded once in the source
is loaded twice in gimple. If it were a HW register which destroys the
device after the 2nd load without an intervening store you'd wrecked
the device ;)
Richard.
Thanks for explaination. I tried to flip order for lhs/rhs in
gimplify_modify_expr co. Issue here is that for some cases we are
relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
like:
typedef const unsigned char _Jv_Utf8Const;
typedef __SIZE_TYPE__ uaddr;
void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special)
{
union {
_Jv_Utf8Const *signature;
uaddr signature_bits;
};
signature = s;
special = signature_bits 1;
signature_bits -= special;
s = signature;
}
So I modified gimplify_self_mod_expr for post-inc/dec so that we use
following sequence
and add it to pre_p for it:
tmp = lhs;
lvalue = tmp (+/-) rhs
*expr_p = tmp;
As I explained this is the wrong place to fix the PR. The issue is not
about self-modifying expressions but about evaluating call argument
side-effects before side-effects of the lhs.
I am testing the attached instead.
Richard.
2012-02-09 Richard Guenther rguent...@suse.de
PR middle-end/48814
* gimplify.c (gimplify_modify_expr): Perform side-effects of
the RHS before those of the LHS.
2012-02-08 Kai Tietz kti...@redhat.com
* gcc.c-torture/execute/pr48814-1.c: New test.
* gcc.c-torture/execute/pr48814-2.c: New test.
* gcc.dg/tree-ssa/assign-1.c: New test.
* gcc.dg/tree-ssa/assign-2.c: New test.
fix-pr48814
Description: Binary data
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/11 Richard Guenther richard.guent...@gmail.com:
count despite being declared volatile and only loaded once in the source
is loaded twice in gimple. If it were a HW register which destroys the
device after the 2nd load without an intervening store you'd wrecked
the device ;)
Richard.
Thanks for explaination. I tried to flip order for lhs/rhs in
gimplify_modify_expr co. Issue here is that for some cases we are
relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
like:
typedef const unsigned char _Jv_Utf8Const;
typedef __SIZE_TYPE__ uaddr;
void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special)
{
union {
_Jv_Utf8Const *signature;
uaddr signature_bits;
};
signature = s;
special = signature_bits 1;
signature_bits -= special;
s = signature;
}
So I modified gimplify_self_mod_expr for post-inc/dec so that we use
following sequence
and add it to pre_p for it:
tmp = lhs;
lvalue = tmp (+/-) rhs
*expr_p = tmp;
As I explained this is the wrong place to fix the PR. The issue is not
about self-modifying expressions but about evaluating call argument
side-effects before side-effects of the lhs.
I am testing the attached instead.
Doesn't work. Btw, Kai, your patch surely breaks things if you put
the lvalue update into the pre queue.
Consider a simple
a[i++] = i;
you gimplify that to
i.0 = i;
D.1709 = i.0;
i = D.1709 + 1;
a[D.1709] = i;
which is wrong.
Seems we are lacking some basic pre-/post-modify testcases ...
Richard.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/2/9 Richard Guenther richard.guent...@gmail.com:
On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/11 Richard Guenther richard.guent...@gmail.com:
count despite being declared volatile and only loaded once in the source
is loaded twice in gimple. If it were a HW register which destroys the
device after the 2nd load without an intervening store you'd wrecked
the device ;)
Richard.
Thanks for explaination. I tried to flip order for lhs/rhs in
gimplify_modify_expr co. Issue here is that for some cases we are
relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
like:
typedef const unsigned char _Jv_Utf8Const;
typedef __SIZE_TYPE__ uaddr;
void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special)
{
union {
_Jv_Utf8Const *signature;
uaddr signature_bits;
};
signature = s;
special = signature_bits 1;
signature_bits -= special;
s = signature;
}
So I modified gimplify_self_mod_expr for post-inc/dec so that we use
following sequence
and add it to pre_p for it:
tmp = lhs;
lvalue = tmp (+/-) rhs
*expr_p = tmp;
As I explained this is the wrong place to fix the PR. The issue is not
about self-modifying expressions but about evaluating call argument
side-effects before side-effects of the lhs.
I am testing the attached instead.
Doesn't work. Btw, Kai, your patch surely breaks things if you put
the lvalue update into the pre queue.
Consider a simple
a[i++] = i;
you gimplify that to
i.0 = i;
D.1709 = i.0;
i = D.1709 + 1;
a[D.1709] = i;
which is wrong.
Seems we are lacking some basic pre-/post-modify testcases ...
Richard.
Why, this should be wrong? In fact C specification just says that the
post-inc has to happen at least before next sequence-point. It
doesn't say that the increment has to happen after evaluation of rhs.
The produced gimple for the following C-code
int arr[128];
void foo (int i)
{
arr[i++] = i;
}
is:
foo (int i)
{
int D.1364;
D.1364 = i;
i = D.1364 + 1;
arr[D.1364] = i;
}
which looks to me from description of the C-specification correct.
Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Thu, Feb 9, 2012 at 3:41 PM, Richard Guenther
richard.guent...@gmail.com wrote:
On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz ktiet...@googlemail.com wrote:
2012/2/9 Richard Guenther richard.guent...@gmail.com:
On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz ktiet...@googlemail.com
wrote:
2012/1/11 Richard Guenther richard.guent...@gmail.com:
count despite being declared volatile and only loaded once in the source
is loaded twice in gimple. If it were a HW register which destroys the
device after the 2nd load without an intervening store you'd wrecked
the device ;)
Richard.
Thanks for explaination. I tried to flip order for lhs/rhs in
gimplify_modify_expr co. Issue here is that for some cases we are
relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
like:
typedef const unsigned char _Jv_Utf8Const;
typedef __SIZE_TYPE__ uaddr;
void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special)
{
union {
_Jv_Utf8Const *signature;
uaddr signature_bits;
};
signature = s;
special = signature_bits 1;
signature_bits -= special;
s = signature;
}
So I modified gimplify_self_mod_expr for post-inc/dec so that we use
following sequence
and add it to pre_p for it:
tmp = lhs;
lvalue = tmp (+/-) rhs
*expr_p = tmp;
As I explained this is the wrong place to fix the PR. The issue is not
about self-modifying expressions but about evaluating call argument
side-effects before side-effects of the lhs.
I am testing the attached instead.
Doesn't work. Btw, Kai, your patch surely breaks things if you put
the lvalue update into the pre queue.
Consider a simple
a[i++] = i;
you gimplify that to
i.0 = i;
D.1709 = i.0;
i = D.1709 + 1;
a[D.1709] = i;
which is wrong.
Seems we are lacking some basic pre-/post-modify testcases ...
Richard.
Why, this should be wrong? In fact C specification just says that the
post-inc has to happen at least before next sequence-point. It
doesn't say that the increment has to happen after evaluation of rhs.
The produced gimple for the following C-code
int arr[128];
void foo (int i)
{
arr[i++] = i;
}
is:
foo (int i)
{
int D.1364;
D.1364 = i;
i = D.1364 + 1;
arr[D.1364] = i;
}
which looks to me from description of the C-specification correct.
Hm, indeed. I'll test the following shorter patch and add the struct-return
volatile testcase.
Works apart from
Running target unix/
FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test
Maybe invalid testcases. Who knows ... same fails happen with your patch.
Richard.
Richard.
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/1/11 Richard Guenther richard.guent...@gmail.com:
count despite being declared volatile and only loaded once in the source
is loaded twice in gimple. If it were a HW register which destroys the
device after the 2nd load without an intervening store you'd wrecked
the device ;)
Richard.
Thanks for explaination. I tried to flip order for lhs/rhs in
gimplify_modify_expr co. Issue here is that for some cases we are
relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
like:
typedef const unsigned char _Jv_Utf8Const;
typedef __SIZE_TYPE__ uaddr;
void maybe_adjust_signature (_Jv_Utf8Const *s, uaddr special)
{
union {
_Jv_Utf8Const *signature;
uaddr signature_bits;
};
signature = s;
special = signature_bits 1;
signature_bits -= special;
s = signature;
}
So I modified gimplify_self_mod_expr for post-inc/dec so that we use
following sequence
and add it to pre_p for it:
tmp = lhs;
lvalue = tmp (+/-) rhs
*expr_p = tmp;
ChangeLog
2012-02-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Move for
postfix-inc/dec the modification in pre and return
temporary with origin value.
2012-02-08 Kai Tietz kti...@redhat.com
* gcc.c-torture/execute/pr48814-1.c: New test.
* gcc.c-torture/execute/pr48814-2.c: New test.
* gcc.dg/tree-ssa/assign-1.c: New test.
* gcc.dg/tree-ssa/assign-2.c: New test.
I did boostrap for all languages (including Ada and Obj-C++) and
regression tests on host x86_64-unknown-linux-gnu. Ok for apply?
Regards,
Kai
Index: gcc/gcc/gimplify.c
===
--- gcc.orig/gcc/gimplify.c
+++ gcc/gcc/gimplify.c
@@ -2197,7 +2197,7 @@ gimplify_self_mod_expr (tree *expr_p, gi
bool want_value)
{
enum tree_code code;
- tree lhs, lvalue, rhs, t1;
+ tree lhs, lvalue, rhs, t1, t2;
gimple_seq post = NULL, *orig_post_p = post_p;
bool postfix;
enum tree_code arith_code;
@@ -2264,20 +2264,23 @@ gimplify_self_mod_expr (tree *expr_p, gi
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
-
- if (postfix)
-{
- gimplify_assign (lvalue, t1, orig_post_p);
- gimplify_seq_add_seq (orig_post_p, post);
- *expr_p = lhs;
- return GS_ALL_DONE;
-}
- else
+ if (!postfix)
{
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
*expr_p = build2 (MODIFY_EXPR, TREE_TYPE (lvalue), lvalue, t1);
return GS_OK;
}
+
+ /* Assign lhs to temporary variable. */
+ t2 = create_tmp_var (TREE_TYPE (lhs), NULL);
+ gimplify_assign (t2, lhs, pre_p);
+ /* Do increment and assign it to lvalue. */
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), t2, rhs);
+ gimplify_assign (lvalue, t1, pre_p);
+
+ gimplify_seq_add_seq (orig_post_p, post);
+ *expr_p = t2;
+ return GS_ALL_DONE;
}
/* If *EXPR_P has a variable sized type, wrap it in a WITH_SIZE_EXPR. */
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-1.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int __attribute__((noinline))
+incr (void)
+{
+ return ++count;
+}
+
+int main()
+{
+ arr[count++] = incr ();
+ if (count != 2 || arr[count] != 3)
+abort ();
+ return 0;
+}
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814-2.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int
+incr (void)
+{
+ return ++count;
+}
+
+int main()
+{
+ arr[count++] = incr ();
+ if (count != 2 || arr[count] != 3)
+abort ();
+ return 0;
+}
Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-1.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options -O2 -fdump-tree-optimized } */
+
+volatile int count;
+void bar(int);
+void foo()
+{
+ bar(count++);
+}
+
+/* { dg-final { scan-tree-dump-times count = 1 optimized} } */
+/* { dg-final { cleanup-tree-dump optimized } } */
Index: gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.dg/tree-ssa/assign-2.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-options -O2 -fdump-tree-optimized } */
+
+volatile int count;
+int arr[4];
+void foo()
+{
+ arr[count++] = 0;
+}
+
+/* { dg-final { scan-tree-dump-times count = 1 optimized} } */
+/* { dg-final { cleanup-tree-dump optimized } } */
+
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/10 Richard Guenther richard.guent...@gmail.com:
On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
Ping
2012/1/8 Kai Tietz ktiet...@googlemail.com:
Hi,
this patch makes sure that for increment of
postfix-increment/decrement we use also orignal lvalue instead of tmp
lhs value for increment. This fixes reported issue about sequence
point in PR/48814
ChangeLog
2012-01-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary
lhs.
Regression tested for x86_64-unknown-linux-gnu for all languages
(including Ada and Obj-C++). Ok for apply?
Regards,
Kai
Index: gimplify.c
===
--- gimplify.c (revision 182720)
+++ gimplify.c (working copy)
@@ -2258,7 +2258,7 @@
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Hi Richard,
Please add testcases. Why does your patch make a difference?
lhs is just the gimplified lvalue.
yes, exactly this makes a big difference for post-inc/dec. I show you
gimple-dump to illustrate this in more detail. I used here -O2 option
with using attached patch.
gcc without that patch produces following gimple for main:
main ()
{
int count.0;
int count.1;
int D.2721;
int D.2725;
int D.2726;
count.0 = count; -- here we store orginal value 'count' for having
array-access-index
D.2721 = incr (); - within that function count gets modified
arr[count.0] = D.2721;
count.1 = count.0 + 1; - Here happens the issue. We increment the
saved value of 'count'
count = count.1; - By this the modification of count in incr() gets void.
...
By the change we make sure to use count's value instead its saved temporary.
Patched gcc produces this gimple:
main ()
{
int count.0;
int count.1;
int D.1718;
int D.1722;
int D.1723;
count.0 = count;
D.1718 = incr ();
arr[count.0] = D.1718;
count.0 = count; -- Reload count.0 for post-inc/dec to use count's
current value
count.1 = count.0 + 1;
count = count.1;
count.0 = count;
Ok, here is the patch with adusted testcase from PR.
With your patch we generate wrong code for
volatile int count;
int arr[4];
void foo()
{
arr[count++] = 0;
}
as we load from count two times instead of once. Similar for
volatile int count;
void bar(int);
void foo()
{
bar(count++);
}
Please add those as testcases for any revised patch you produce.
Thanks,
Richard.
ChangeLog
2012-01-10 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary lhs.
Regression tested for all languages (including Ada and Obj-C++). Ok for
apply?
Regards,
Kai
2012-01-10 Kai Tietz kti...@redhat.com
* gcc.c-torture/execute/pr48814.c: New test.
Index: gcc/gcc/gimplify.c
===
--- gcc.orig/gcc/gimplify.c
+++ gcc/gcc/gimplify.c
@@ -2258,7 +2258,7 @@ gimplify_self_mod_expr (tree *expr_p, gi
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int __attribute__((noinline))
+incr (void)
+{
+ return ++count;
+}
+
+int main()
+{
+ arr[count++] = incr ();
+ if (count != 2 || arr[count] != 3)
+ abort ();
+ return 0;
+}
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/10 Richard Guenther richard.guent...@gmail.com:
On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
Ping
2012/1/8 Kai Tietz ktiet...@googlemail.com:
Hi,
this patch makes sure that for increment of
postfix-increment/decrement we use also orignal lvalue instead of tmp
lhs value for increment. This fixes reported issue about sequence
point in PR/48814
ChangeLog
2012-01-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary
lhs.
Regression tested for x86_64-unknown-linux-gnu for all languages
(including Ada and Obj-C++). Ok for apply?
Regards,
Kai
Index: gimplify.c
===
--- gimplify.c (revision 182720)
+++ gimplify.c (working copy)
@@ -2258,7 +2258,7 @@
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Hi Richard,
Please add testcases. Why does your patch make a difference?
lhs is just the gimplified lvalue.
yes, exactly this makes a big difference for post-inc/dec. I show you
gimple-dump to illustrate this in more detail. I used here -O2 option
with using attached patch.
gcc without that patch produces following gimple for main:
main ()
{
int count.0;
int count.1;
int D.2721;
int D.2725;
int D.2726;
count.0 = count; -- here we store orginal value 'count' for having
array-access-index
D.2721 = incr (); - within that function count gets modified
arr[count.0] = D.2721;
count.1 = count.0 + 1; - Here happens the issue. We increment the
saved value of 'count'
count = count.1; - By this the modification of count in incr() gets void.
...
By the change we make sure to use count's value instead its saved temporary.
Patched gcc produces this gimple:
main ()
{
int count.0;
int count.1;
int D.1718;
int D.1722;
int D.1723;
count.0 = count;
D.1718 = incr ();
arr[count.0] = D.1718;
count.0 = count; -- Reload count.0 for post-inc/dec to use count's
current value
count.1 = count.0 + 1;
count = count.1;
count.0 = count;
Ok, here is the patch with adusted testcase from PR.
With your patch we generate wrong code for
volatile int count;
int arr[4];
void foo()
{
arr[count++] = 0;
}
as we load from count two times instead of once. Similar for
volatile int count;
void bar(int);
void foo()
{
bar(count++);
}
Please add those as testcases for any revised patch you produce.
I think a proper fix involves changing the order we gimplify the
lhs/rhs for calls.
Thanks,
Richard.
ChangeLog
2012-01-10 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary lhs.
Regression tested for all languages (including Ada and Obj-C++). Ok for
apply?
Regards,
Kai
2012-01-10 Kai Tietz kti...@redhat.com
* gcc.c-torture/execute/pr48814.c: New test.
Index: gcc/gcc/gimplify.c
===
--- gcc.orig/gcc/gimplify.c
+++ gcc/gcc/gimplify.c
@@ -2258,7 +2258,7 @@ gimplify_self_mod_expr (tree *expr_p, gi
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int __attribute__((noinline))
+incr (void)
+{
+ return ++count;
+}
+
+int main()
+{
+ arr[count++] = incr ();
+ if (count != 2 || arr[count] != 3)
+ abort ();
+ return 0;
+}
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/1/11 Richard Guenther richard.guent...@gmail.com:
On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/10 Richard Guenther richard.guent...@gmail.com:
On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com
wrote:
Ping
2012/1/8 Kai Tietz ktiet...@googlemail.com:
Hi,
this patch makes sure that for increment of
postfix-increment/decrement we use also orignal lvalue instead of tmp
lhs value for increment. This fixes reported issue about sequence
point in PR/48814
ChangeLog
2012-01-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary
lhs.
Regression tested for x86_64-unknown-linux-gnu for all languages
(including Ada and Obj-C++). Ok for apply?
Regards,
Kai
Index: gimplify.c
===
--- gimplify.c (revision 182720)
+++ gimplify.c (working copy)
@@ -2258,7 +2258,7 @@
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Hi Richard,
Please add testcases. Why does your patch make a difference?
lhs is just the gimplified lvalue.
yes, exactly this makes a big difference for post-inc/dec. I show you
gimple-dump to illustrate this in more detail. I used here -O2 option
with using attached patch.
gcc without that patch produces following gimple for main:
main ()
{
int count.0;
int count.1;
int D.2721;
int D.2725;
int D.2726;
count.0 = count; -- here we store orginal value 'count' for having
array-access-index
D.2721 = incr (); - within that function count gets modified
arr[count.0] = D.2721;
count.1 = count.0 + 1; - Here happens the issue. We increment the
saved value of 'count'
count = count.1; - By this the modification of count in incr() gets void.
...
By the change we make sure to use count's value instead its saved temporary.
Patched gcc produces this gimple:
main ()
{
int count.0;
int count.1;
int D.1718;
int D.1722;
int D.1723;
count.0 = count;
D.1718 = incr ();
arr[count.0] = D.1718;
count.0 = count; -- Reload count.0 for post-inc/dec to use count's
current value
count.1 = count.0 + 1;
count = count.1;
count.0 = count;
Ok, here is the patch with adusted testcase from PR.
With your patch we generate wrong code for
volatile int count;
int arr[4];
void foo()
{
arr[count++] = 0;
}
as we load from count two times instead of once. Similar for
volatile int count;
void bar(int);
void foo()
{
bar(count++);
}
Please add those as testcases for any revised patch you produce.
I think a proper fix involves changing the order we gimplify the
lhs/rhs for calls.
Hmm, I don't see actual wrong code here. But I might missed something in spec.
For exampl1 we get:
foo ()
{
volatile int count.0;
volatile int count.1;
volatile int count.2;
count.0 = count;
arr[count.0] = 0;
count.1 = count;
count.2 = count.1 + 1;
count = count.2;
}
and here is no wrong code AFAICS.
For second example we get:
foo ()
{
volatile int count.0;
volatile int count.1;
volatile int count.2;
volatile int count.3;
count.0 = count;
count.3 = count.0;
count.1 = count;
count.2 = count.1 + 1;
count = count.2;
bar (count.3);
}
Here we don't have wrong code either AFAICS. Passed argument to bar is
the value before increment, and count get incremented by 1 before call
of bar, which is right.
Thanks for more details,
Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Wed, Jan 11, 2012 at 11:19 AM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/11 Richard Guenther richard.guent...@gmail.com:
On Wed, Jan 11, 2012 at 11:05 AM, Richard Guenther
richard.guent...@gmail.com wrote:
On Tue, Jan 10, 2012 at 11:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
2012/1/10 Richard Guenther richard.guent...@gmail.com:
On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com
wrote:
Ping
2012/1/8 Kai Tietz ktiet...@googlemail.com:
Hi,
this patch makes sure that for increment of
postfix-increment/decrement we use also orignal lvalue instead of tmp
lhs value for increment. This fixes reported issue about sequence
point in PR/48814
ChangeLog
2012-01-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary
lhs.
Regression tested for x86_64-unknown-linux-gnu for all languages
(including Ada and Obj-C++). Ok for apply?
Regards,
Kai
Index: gimplify.c
===
--- gimplify.c (revision 182720)
+++ gimplify.c (working copy)
@@ -2258,7 +2258,7 @@
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Hi Richard,
Please add testcases. Why does your patch make a difference?
lhs is just the gimplified lvalue.
yes, exactly this makes a big difference for post-inc/dec. I show you
gimple-dump to illustrate this in more detail. I used here -O2 option
with using attached patch.
gcc without that patch produces following gimple for main:
main ()
{
int count.0;
int count.1;
int D.2721;
int D.2725;
int D.2726;
count.0 = count; -- here we store orginal value 'count' for having
array-access-index
D.2721 = incr (); - within that function count gets modified
arr[count.0] = D.2721;
count.1 = count.0 + 1; - Here happens the issue. We increment the
saved value of 'count'
count = count.1; - By this the modification of count in incr() gets void.
...
By the change we make sure to use count's value instead its saved
temporary.
Patched gcc produces this gimple:
main ()
{
int count.0;
int count.1;
int D.1718;
int D.1722;
int D.1723;
count.0 = count;
D.1718 = incr ();
arr[count.0] = D.1718;
count.0 = count; -- Reload count.0 for post-inc/dec to use count's
current value
count.1 = count.0 + 1;
count = count.1;
count.0 = count;
Ok, here is the patch with adusted testcase from PR.
With your patch we generate wrong code for
volatile int count;
int arr[4];
void foo()
{
arr[count++] = 0;
}
as we load from count two times instead of once. Similar for
volatile int count;
void bar(int);
void foo()
{
bar(count++);
}
Please add those as testcases for any revised patch you produce.
I think a proper fix involves changing the order we gimplify the
lhs/rhs for calls.
Hmm, I don't see actual wrong code here. But I might missed something in
spec.
For exampl1 we get:
foo ()
{
volatile int count.0;
volatile int count.1;
volatile int count.2;
count.0 = count;
arr[count.0] = 0;
count.1 = count;
count.2 = count.1 + 1;
count = count.2;
}
count despite being declared volatile and only loaded once in the source
is loaded twice in gimple. If it were a HW register which destroys the
device after the 2nd load without an intervening store you'd wrecked
the device ;)
Richard.
and here is no wrong code AFAICS.
For second example we get:
foo ()
{
volatile int count.0;
volatile int count.1;
volatile int count.2;
volatile int count.3;
count.0 = count;
count.3 = count.0;
count.1 = count;
count.2 = count.1 + 1;
count = count.2;
bar (count.3);
}
Here we don't have wrong code either AFAICS. Passed argument to bar is
the value before increment, and count get incremented by 1 before call
of bar, which is right.
Thanks for more details,
Kai
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
Ping
2012/1/8 Kai Tietz ktiet...@googlemail.com:
Hi,
this patch makes sure that for increment of
postfix-increment/decrement we use also orignal lvalue instead of tmp
lhs value for increment. This fixes reported issue about sequence
point in PR/48814
ChangeLog
2012-01-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary
lhs.
Regression tested for x86_64-unknown-linux-gnu for all languages
(including Ada and Obj-C++). Ok for apply?
Regards,
Kai
Index: gimplify.c
===
--- gimplify.c (revision 182720)
+++ gimplify.c (working copy)
@@ -2258,7 +2258,7 @@
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Please add testcases. Why does your patch make a difference?
lhs is just the gimplified lvalue.
Richard.
--
| (\_/) This is Bunny. Copy and paste
| (='.'=) Bunny into your signature to help
| ()_() him gain world domination
Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result
2012/1/10 Richard Guenther richard.guent...@gmail.com:
On Tue, Jan 10, 2012 at 10:58 AM, Kai Tietz ktiet...@googlemail.com wrote:
Ping
2012/1/8 Kai Tietz ktiet...@googlemail.com:
Hi,
this patch makes sure that for increment of
postfix-increment/decrement we use also orignal lvalue instead of tmp
lhs value for increment. This fixes reported issue about sequence
point in PR/48814
ChangeLog
2012-01-08 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary
lhs.
Regression tested for x86_64-unknown-linux-gnu for all languages
(including Ada and Obj-C++). Ok for apply?
Regards,
Kai
Index: gimplify.c
===
--- gimplify.c (revision 182720)
+++ gimplify.c (working copy)
@@ -2258,7 +2258,7 @@
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Hi Richard,
Please add testcases. Why does your patch make a difference?
lhs is just the gimplified lvalue.
yes, exactly this makes a big difference for post-inc/dec. I show you
gimple-dump to illustrate this in more detail. I used here -O2 option
with using attached patch.
gcc without that patch produces following gimple for main:
main ()
{
int count.0;
int count.1;
int D.2721;
int D.2725;
int D.2726;
count.0 = count; -- here we store orginal value 'count' for having
array-access-index
D.2721 = incr (); - within that function count gets modified
arr[count.0] = D.2721;
count.1 = count.0 + 1; - Here happens the issue. We increment the
saved value of 'count'
count = count.1; - By this the modification of count in incr() gets void.
...
By the change we make sure to use count's value instead its saved temporary.
Patched gcc produces this gimple:
main ()
{
int count.0;
int count.1;
int D.1718;
int D.1722;
int D.1723;
count.0 = count;
D.1718 = incr ();
arr[count.0] = D.1718;
count.0 = count; -- Reload count.0 for post-inc/dec to use count's
current value
count.1 = count.0 + 1;
count = count.1;
count.0 = count;
Ok, here is the patch with adusted testcase from PR.
ChangeLog
2012-01-10 Kai Tietz kti...@redhat.com
PR middle-end/48814
* gimplify.c (gimplify_self_mod_expr): Use for
postfix-inc/dec lvalue instead of temporary lhs.
Regression tested for all languages (including Ada and Obj-C++). Ok for apply?
Regards,
Kai
2012-01-10 Kai Tietz kti...@redhat.com
* gcc.c-torture/execute/pr48814.c: New test.
Index: gcc/gcc/gimplify.c
===
--- gcc.orig/gcc/gimplify.c
+++ gcc/gcc/gimplify.c
@@ -2258,7 +2258,7 @@ gimplify_self_mod_expr (tree *expr_p, gi
arith_code = POINTER_PLUS_EXPR;
}
- t1 = build2 (arith_code, TREE_TYPE (*expr_p), lhs, rhs);
+ t1 = build2 (arith_code, TREE_TYPE (*expr_p), lvalue, rhs);
if (postfix)
{
Index: gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c
===
--- /dev/null
+++ gcc/gcc/testsuite/gcc.c-torture/execute/pr48814.c
@@ -0,0 +1,18 @@
+extern void abort (void);
+
+int arr[] = {1,2,3,4};
+int count = 0;
+
+int __attribute__((noinline))
+incr (void)
+{
+ return ++count;
+}
+
+int main()
+{
+ arr[count++] = incr ();
+ if (count != 2 || arr[count] != 3)
+abort ();
+ return 0;
+}
