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
> ===================================================================
> --- /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" } } */
> +
