diff mbox

RFA (gimplify): PATCH to implement C++ order of evaluation paper

Message ID CADzB+2mdcgwCQAsUAuynyhbzYQdNdJJ5YhKbPzJV-vEgWBFb3A@mail.gmail.com
State New
Headers show

Commit Message

Jason Merrill June 16, 2016, 3:28 p.m. UTC 
On Wed, Jun 15, 2016 at 6:30 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, Jun 14, 2016 at 10:15 PM, Jason Merrill <jason@redhat.com> wrote:
>> As discussed in bug 71104, the C++ P0145 proposal specifies the evaluation
>> order of certain operations:
>>
>> 1. a.b
>> 2. a->b
>> 3. a->*b
>> 4. a(b1, b2, b3)
>> 5. b @= a
>> 6. a[b]
>> 7. a << b
>> 8. a >> b
>>
>> The second patch introduces a flag -fargs-in-order to control whether these
>> orders are enforced on calls.  -fargs-in-order=1 enforces all but the
>> ordering between function arguments in #4.
>>
>> The first patch implements #5 for the built-in assignment operator by
>> changing the order of gimplification of MODIFY_EXPR in the back end, as
>> richi was also thinking about doing to fix 71104.  This runs into problems
>> with DECL_VALUE_EXPR variables, where is_gimple_reg can be true before
>> gimplification and false afterward, so he checks for this situation in
>> rhs_predicate_for.  richi, you said you were still working on 71104; is this
>> patch OK to put in for now, or should I wait for something better?

> I wasn't too happy about the rhs_predicate_for change and I was also worried
> about generating a lot less optimal GIMPLE due to evaluating the predicate
> on un-gimplified *to_p.

We can try to be more clever about recognizing things that will
gimplify to a reg.  How does this patch look?

> I wondered if we should simply gimplify *from_p
> with is_gimple_mem_rhs_or_call unconditionally, then gimplify *to_p
> and after that if (unmodified) rhs_predicate_for (*to_p) is !=
> is_gimple_mem_rhs_or_call re-gimplify *from_p to avoid this.  That should also avoid changing
> rhs_predicate_for.

The problem with this approach is that gimplification is destructive;
you can't just throw away the first sequence and gimplify again.  For
instance, SAVE_EXPRs are clobbered the first time they are seen in
gimplification.

> Not sure if that solves whatever you were running into with OpenMP.
>
> I simply didn't have found the time to experiment with the above or even
> validate my fear by say comparing .gimple dumps of cc1 files with/without
> the gimplification order change.

Looking through the gimple dumps for optabs.c and c-common.c with this
patch I don't see any increase in temporaries, but I do see some
improved locality such that we initialize a pointer temporary just
before assigning to one of its fields rather than initializing it
before doing all the value computation, e.g.

before:
-      _14 = *node;
-      _15 = contains_struct_check (_14, 1, "../../../gcc/gcc/c-family/c-common.
c", 7672, &__FUNCTION__);
...lots...
-      _15->typed.type = _56;

after:
+      _55 = *node;
+      _56 = contains_struct_check (_55, 1,
"../../../gcc/gcc/c-family/c-common.c", 7672, &__FUNCTION__);
+      _56->typed.type = _54;

Is this version of the patch OK?

Jason
commit 50495a102be99950002b0cc9f824fcb90cdf65fb
Author: Jason Merrill <jason@redhat.com>
Date:   Thu Jun 16 01:25:02 2016 -0400

    	P0145R2: Refining Expression Order for C++ (assignment)
    
    	* gimplify.c (will_be_gimple_reg): New.
    	(rhs_predicate_for): Use it.
    	(gimplify_modify_expr): Gimplify RHS first.

Comments

Jakub Jelinek June 16, 2016, 4:15 p.m. UTC | #1 
On Thu, Jun 16, 2016 at 11:28:48AM -0400, Jason Merrill wrote:
>  gimple_predicate
>  rhs_predicate_for (tree lhs)
>  {
> -  if (is_gimple_reg (lhs))
> +  if (will_be_gimple_reg (lhs))
>      return is_gimple_reg_rhs_or_call;
>    else
>      return is_gimple_mem_rhs_or_call;
> @@ -4778,10 +4811,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>       that is what we must do here.  */
>    maybe_with_size_expr (from_p);
>  
> -  ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
> -  if (ret == GS_ERROR)
> -    return ret;
> -
>    /* As a special case, we have to temporarily allow for assignments
>       with a CALL_EXPR on the RHS.  Since in GIMPLE a function call is
>       a toplevel statement, when gimplifying the GENERIC expression
> @@ -4799,6 +4828,10 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>    if (ret == GS_ERROR)
>      return ret;
>  
> +  ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
> +  if (ret == GS_ERROR)
> +    return ret;
> +
>    /* In case of va_arg internal fn wrappped in a WITH_SIZE_EXPR, add the type
>       size as argument to the call.  */
>    if (TREE_CODE (*from_p) == WITH_SIZE_EXPR)

I wonder if instead of trying to guess early what we'll gimplify into it
wouldn't be better to gimplify *from_p twice, first time with a predicate
that would assume *to_p could be gimplified into is_gimple_ref, but
guarantee there are no side-effects (so that those aren't evaluated
after lhs side-effects), and second time if needed (if *to_p didn't end up
being is_gimple_reg).  So something like a new predicate like:

static bool
is_whatever (tree t)
{
  /* For calls, as there are side-effects, assume lhs might not be
     is_gimple_reg.  */
  if (TREE_CODE (t) == CALL_EXPR && is_gimple_reg_type (TREE_TYPE (t)))
    return is_gimple_val (t);
  /* For other side effects, also make sure those are evaluated before
     side-effects in lhs.  */
  if (TREE_THIS_VOLATILE (t))
    return is_gimple_mem_rhs_or_call (t);
  /* Otherwise, optimistically assume lhs will be is_gimple_reg.  */
  return is_gimple_reg_rhs_or_call (t);
}

and then do in gimplify_modify_expr:
  ret = gimplify_expr (from_p, pre_p, post_p,
		       is_gimple_reg (*to_p)
		       ? is_gimple_reg_rhs_or_call : is_whatever,
                       fb_rvalue);
  if (ret == GS_ERROR)
    return ret;

  ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
  if (ret == GS_ERROR)
    return ret;

  if (!is_gimple_reg (*to_p) && !is_gimple_mem_rhs_or_call (*from_p))
    {
      ret = gimplify_expr (from_p, pre_p, post_p, is_gimple_mem_rhs_or_call,
			   fb_rvalue);
      if (ret == GS_ERROR)
	return ret;
    }

Or if you want to guess if *to_p will be is_gimple_reg or not after
gimplification, do it just very conservatively and let the more difficult
to predict cases handled worst case by forcing something into a temporary
with the above code.

	Jakub
Jakub Jelinek June 16, 2016, 4:19 p.m. UTC | #2 
On Thu, Jun 16, 2016 at 06:15:34PM +0200, Jakub Jelinek wrote:
> and then do in gimplify_modify_expr:
>   ret = gimplify_expr (from_p, pre_p, post_p,
> 		       is_gimple_reg (*to_p)

^^^ of course even this is a prediction and wrong one for
DECL_HAS_VALUE_EXPR_Ps.  Conservative would be is_whatever always.

> 		       ? is_gimple_reg_rhs_or_call : is_whatever,
>                        fb_rvalue);
>   if (ret == GS_ERROR)
>     return ret;
> 
>   ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>   if (ret == GS_ERROR)
>     return ret;
> 
>   if (!is_gimple_reg (*to_p) && !is_gimple_mem_rhs_or_call (*from_p))
>     {
>       ret = gimplify_expr (from_p, pre_p, post_p, is_gimple_mem_rhs_or_call,
> 			   fb_rvalue);
>       if (ret == GS_ERROR)
> 	return ret;
>     }
> 
> Or if you want to guess if *to_p will be is_gimple_reg or not after
> gimplification, do it just very conservatively and let the more difficult
> to predict cases handled worst case by forcing something into a temporary
> with the above code.

	Jakub
Richard Biener June 28, 2016, 2 p.m. UTC | #3 
On Thu, Jun 16, 2016 at 6:15 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Jun 16, 2016 at 11:28:48AM -0400, Jason Merrill wrote:
>>  gimple_predicate
>>  rhs_predicate_for (tree lhs)
>>  {
>> -  if (is_gimple_reg (lhs))
>> +  if (will_be_gimple_reg (lhs))
>>      return is_gimple_reg_rhs_or_call;
>>    else
>>      return is_gimple_mem_rhs_or_call;
>> @@ -4778,10 +4811,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>>       that is what we must do here.  */
>>    maybe_with_size_expr (from_p);
>>
>> -  ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>> -  if (ret == GS_ERROR)
>> -    return ret;
>> -
>>    /* As a special case, we have to temporarily allow for assignments
>>       with a CALL_EXPR on the RHS.  Since in GIMPLE a function call is
>>       a toplevel statement, when gimplifying the GENERIC expression
>> @@ -4799,6 +4828,10 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>>    if (ret == GS_ERROR)
>>      return ret;
>>
>> +  ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>> +  if (ret == GS_ERROR)
>> +    return ret;
>> +
>>    /* In case of va_arg internal fn wrappped in a WITH_SIZE_EXPR, add the type
>>       size as argument to the call.  */
>>    if (TREE_CODE (*from_p) == WITH_SIZE_EXPR)
>
> I wonder if instead of trying to guess early what we'll gimplify into it
> wouldn't be better to gimplify *from_p twice, first time with a predicate
> that would assume *to_p could be gimplified into is_gimple_ref, but
> guarantee there are no side-effects (so that those aren't evaluated
> after lhs side-effects), and second time if needed (if *to_p didn't end up
> being is_gimple_reg).  So something like a new predicate like:

Yes, that is what I was suggesting.

Richard.

> static bool
> is_whatever (tree t)
> {
>   /* For calls, as there are side-effects, assume lhs might not be
>      is_gimple_reg.  */
>   if (TREE_CODE (t) == CALL_EXPR && is_gimple_reg_type (TREE_TYPE (t)))
>     return is_gimple_val (t);
>   /* For other side effects, also make sure those are evaluated before
>      side-effects in lhs.  */
>   if (TREE_THIS_VOLATILE (t))
>     return is_gimple_mem_rhs_or_call (t);
>   /* Otherwise, optimistically assume lhs will be is_gimple_reg.  */
>   return is_gimple_reg_rhs_or_call (t);
> }
>
> and then do in gimplify_modify_expr:
>   ret = gimplify_expr (from_p, pre_p, post_p,
>                        is_gimple_reg (*to_p)
>                        ? is_gimple_reg_rhs_or_call : is_whatever,
>                        fb_rvalue);
>   if (ret == GS_ERROR)
>     return ret;
>
>   ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
>   if (ret == GS_ERROR)
>     return ret;
>
>   if (!is_gimple_reg (*to_p) && !is_gimple_mem_rhs_or_call (*from_p))
>     {
>       ret = gimplify_expr (from_p, pre_p, post_p, is_gimple_mem_rhs_or_call,
>                            fb_rvalue);
>       if (ret == GS_ERROR)
>         return ret;
>     }
>
> Or if you want to guess if *to_p will be is_gimple_reg or not after
> gimplification, do it just very conservatively and let the more difficult
> to predict cases handled worst case by forcing something into a temporary
> with the above code.
>
>         Jakub
diff mbox

Patch

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index ae8b4fc..5d51d64 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -3802,12 +3802,45 @@  gimplify_init_ctor_eval (tree object, vec<constructor_elt, va_gc> *elts,
     }
 }
 
+/* Return true if LHS will satisfy is_gimple_reg after gimplification.  */
+
+static bool
+will_be_gimple_reg (tree lhs)
+{
+  while (true)
+    switch (TREE_CODE (lhs))
+      {
+      case COMPOUND_EXPR:
+	lhs = TREE_OPERAND (lhs, 1);
+	break;
+
+      case INIT_EXPR:
+      case MODIFY_EXPR:
+      case PREINCREMENT_EXPR:
+      case PREDECREMENT_EXPR:
+	lhs = TREE_OPERAND (lhs, 0);
+	break;
+
+      case VAR_DECL:
+      case PARM_DECL:
+      case RESULT_DECL:
+	if (DECL_HAS_VALUE_EXPR_P (lhs))
+	  {
+	    lhs = DECL_VALUE_EXPR (lhs);
+	    break;
+	  }
+	/* else fall through.  */
+      default:
+	return is_gimple_reg (lhs);
+      }
+}
+
 /* Return the appropriate RHS predicate for this LHS.  */
 
 gimple_predicate
 rhs_predicate_for (tree lhs)
 {
-  if (is_gimple_reg (lhs))
+  if (will_be_gimple_reg (lhs))
     return is_gimple_reg_rhs_or_call;
   else
     return is_gimple_mem_rhs_or_call;
@@ -4778,10 +4811,6 @@  gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
      that is what we must do here.  */
   maybe_with_size_expr (from_p);
 
-  ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
-  if (ret == GS_ERROR)
-    return ret;
-
   /* As a special case, we have to temporarily allow for assignments
      with a CALL_EXPR on the RHS.  Since in GIMPLE a function call is
      a toplevel statement, when gimplifying the GENERIC expression
@@ -4799,6 +4828,10 @@  gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   if (ret == GS_ERROR)
     return ret;
 
+  ret = gimplify_expr (to_p, pre_p, post_p, is_gimple_lvalue, fb_lvalue);
+  if (ret == GS_ERROR)
+    return ret;
+
   /* In case of va_arg internal fn wrappped in a WITH_SIZE_EXPR, add the type
      size as argument to the call.  */
   if (TREE_CODE (*from_p) == WITH_SIZE_EXPR)
diff --git a/gcc/testsuite/g++.dg/cpp1z/eval-order3.C b/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
index 15df903..d351219 100644
--- a/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
+++ b/gcc/testsuite/g++.dg/cpp1z/eval-order3.C
@@ -84,7 +84,7 @@  template <class T> void f()
 
   // b @= a
   aref(19)=A(18);
-  //iref(21)=f(20);
+  iref(21)=f(20);
   aref(23)+=f(22);
   last = 0;
 
@@ -123,7 +123,7 @@  void g()
 
   // b @= a
   aref(19)=A(18);
-  //iref(21)=f(20);
+  iref(21)=f(20);
   aref(23)+=f(22);
   last = 0;