This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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 <ktietz70@googlemail.com> wrote:
> 2012/1/11 Richard Guenther <richard.guenther@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 ?<ktietz@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 ?<ktietz@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" } } */
> +


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]