c++, v2: Add support for __real__/__imag__ modifications in constant expressions [PR88174]

  On Fri, Jun 10, 2022 at 09:57:06PM +0200, Jakub Jelinek via Gcc-patches wrote:
> On Fri, Jun 10, 2022 at 01:27:28PM -0400, Jason Merrill wrote:
> > Doesn't this assert mean that complex_expr will always be == valp?
> 
> No, even when handling the pushed *PART_EXPR, it will set
> valp = &TREE_OPERAND (*valp, index != integer_zero_node);
> So, valp will be either &TREE_OPERAND (*complex_expr, 0)
> or &TREE_OPERAND (*complex_expr, 1).
> As *valp = init; is what is usually then stored and we want to store there
> the scalar.
> 
> > I don't understand this block; shouldn't valp point to the real or imag part
> > of the complex number at this point?  How could complex_part be set without
> > us handling the complex case in the loop already?
> 
> Because for most references, the code will do:
>       vec_safe_push (ctors, *valp);
>       vec_safe_push (indexes, index);
> I chose not to do this for *PART_EXPR, because the COMPLEX_EXPR isn't a
> CONSTRUCTOR and code later on e.g. walks all the ctors and accesses
> CONSTRUCTOR_NO_CLEARING on them etc.  As the *PART_EXPR is asserted to
> be outermost only, complex_expr is a variant of that ctors push and
> complex_part of the indexes.
> The reason for the above if is just in case the evaluation of the rhs
> of the store would store to the complex and could e.g. make it a COMPLEX_CST
> again.
> 
> > I might have added the COMPLEX_EXPR to ctors instead of a separate variable,
> > but this is fine too.
> 
> See above.
> The COMPLEX_EXPR needs special handling (conversion into COMPLEX_CST if it
> is constant) anyway.

Here is a variant patch which pushes even the *PART_EXPR related entries
into ctors and indexes vectors, so it doesn't need to use extra variables
for the complex stuff.

2022-06-17  Jakub Jelinek  <jakub@redhat.com>

	PR c++/88174
	* constexpr.cc (cxx_eval_store_expression): Handle REALPART_EXPR
	and IMAGPART_EXPR.  Change ctors from releasing_vec to
	auto_vec<tree *>, adjust all uses.

	* g++.dg/cpp1y/constexpr-complex1.C: New test.



	Jakub
  

On 6/17/22 13:06, Jakub Jelinek wrote:
> On Fri, Jun 10, 2022 at 09:57:06PM +0200, Jakub Jelinek via Gcc-patches wrote:
>> On Fri, Jun 10, 2022 at 01:27:28PM -0400, Jason Merrill wrote:
>>> Doesn't this assert mean that complex_expr will always be == valp?
>>
>> No, even when handling the pushed *PART_EXPR, it will set
>> valp = &TREE_OPERAND (*valp, index != integer_zero_node);
>> So, valp will be either &TREE_OPERAND (*complex_expr, 0)
>> or &TREE_OPERAND (*complex_expr, 1).
>> As *valp = init; is what is usually then stored and we want to store there
>> the scalar.
>>
>>> I don't understand this block; shouldn't valp point to the real or imag part
>>> of the complex number at this point?  How could complex_part be set without
>>> us handling the complex case in the loop already?
>>
>> Because for most references, the code will do:
>>        vec_safe_push (ctors, *valp);
>>        vec_safe_push (indexes, index);
>> I chose not to do this for *PART_EXPR, because the COMPLEX_EXPR isn't a
>> CONSTRUCTOR and code later on e.g. walks all the ctors and accesses
>> CONSTRUCTOR_NO_CLEARING on them etc.  As the *PART_EXPR is asserted to
>> be outermost only, complex_expr is a variant of that ctors push and
>> complex_part of the indexes.
>> The reason for the above if is just in case the evaluation of the rhs
>> of the store would store to the complex and could e.g. make it a COMPLEX_CST
>> again.
>>
>>> I might have added the COMPLEX_EXPR to ctors instead of a separate variable,
>>> but this is fine too.
>>
>> See above.
>> The COMPLEX_EXPR needs special handling (conversion into COMPLEX_CST if it
>> is constant) anyway.
> 
> Here is a variant patch which pushes even the *PART_EXPR related entries
> into ctors and indexes vectors, so it doesn't need to use extra variables
> for the complex stuff.

Thanks.

> 2022-06-17  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/88174
> 	* constexpr.cc (cxx_eval_store_expression): Handle REALPART_EXPR
> 	and IMAGPART_EXPR.  Change ctors from releasing_vec to
> 	auto_vec<tree *>, adjust all uses.
> 
> 	* g++.dg/cpp1y/constexpr-complex1.C: New test.
> 
> --- gcc/cp/constexpr.cc.jj	2022-06-09 17:42:23.606243920 +0200
> +++ gcc/cp/constexpr.cc	2022-06-17 18:59:54.809208997 +0200
> @@ -5714,6 +5714,20 @@ cxx_eval_store_expression (const constex
>   	  }
>   	  break;
>   
> +	case REALPART_EXPR:
> +	  gcc_assert (probe == target);
> +	  vec_safe_push (refs, probe);
> +	  vec_safe_push (refs, TREE_TYPE (probe));
> +	  probe = TREE_OPERAND (probe, 0);
> +	  break;
> +
> +	case IMAGPART_EXPR:
> +	  gcc_assert (probe == target);
> +	  vec_safe_push (refs, probe);
> +	  vec_safe_push (refs, TREE_TYPE (probe));
> +	  probe = TREE_OPERAND (probe, 0);
> +	  break;
> +
>   	default:
>   	  if (evaluated)
>   	    object = probe;
> @@ -5752,7 +5766,8 @@ cxx_eval_store_expression (const constex
>     type = TREE_TYPE (object);
>     bool no_zero_init = true;
>   
> -  releasing_vec ctors, indexes;
> +  auto_vec<tree *> ctors;
> +  releasing_vec indexes;
>     auto_vec<int> index_pos_hints;
>     bool activated_union_member_p = false;
>     bool empty_base = false;
> @@ -5792,14 +5807,36 @@ cxx_eval_store_expression (const constex
>   	  *valp = ary_ctor;
>   	}
>   
> -      /* If the value of object is already zero-initialized, any new ctors for
> -	 subobjects will also be zero-initialized.  */
> -      no_zero_init = CONSTRUCTOR_NO_CLEARING (*valp);
> -
>         enum tree_code code = TREE_CODE (type);
>         tree reftype = refs->pop();
>         tree index = refs->pop();
>   
> +      if (code == COMPLEX_TYPE)
> +	{
> +	  if (TREE_CODE (*valp) == COMPLEX_CST)
> +	    *valp = build2 (COMPLEX_EXPR, type, TREE_REALPART (*valp),
> +			    TREE_IMAGPART (*valp));
> +	  else if (TREE_CODE (*valp) == CONSTRUCTOR
> +		   && CONSTRUCTOR_NELTS (*valp) == 0
> +		   && CONSTRUCTOR_NO_CLEARING (*valp))
> +	    {
> +	      tree r = build_constructor (reftype, NULL);
> +	      CONSTRUCTOR_NO_CLEARING (r) = 1;
> +	      *valp = build2 (COMPLEX_EXPR, type, r, r);
> +	    }
> +	  gcc_assert (TREE_CODE (*valp) == COMPLEX_EXPR);
> +	  ctors.safe_push (valp);
> +	  vec_safe_push (indexes, index);
> +	  valp = &TREE_OPERAND (*valp, TREE_CODE (index) == IMAGPART_EXPR);
> +	  gcc_checking_assert (refs->is_empty ());
> +	  type = reftype;
> +	  break;
> +	}
> +
> +      /* If the value of object is already zero-initialized, any new ctors for
> +	 subobjects will also be zero-initialized.  */
> +      no_zero_init = CONSTRUCTOR_NO_CLEARING (*valp);
> +
>         if (code == RECORD_TYPE && is_empty_field (index))
>   	/* Don't build a sub-CONSTRUCTOR for an empty base or field, as they
>   	   have no data and might have an offset lower than previously declared
> @@ -5842,7 +5879,7 @@ cxx_eval_store_expression (const constex
>   	  no_zero_init = true;
>   	}
>   
> -      vec_safe_push (ctors, *valp);
> +      ctors.safe_push (valp);
>         vec_safe_push (indexes, index);
>   
>         constructor_elt *cep
> @@ -5904,11 +5941,11 @@ cxx_eval_store_expression (const constex
>   	     semantics are not applied on an object under construction.
>   	     They come into effect when the constructor for the most
>   	     derived object ends."  */
> -	  for (tree elt : *ctors)
> +	  for (tree *elt : ctors)
>   	    if (same_type_ignoring_top_level_qualifiers_p
> -		(TREE_TYPE (const_object_being_modified), TREE_TYPE (elt)))
> +		(TREE_TYPE (const_object_being_modified), TREE_TYPE (*elt)))
>   	      {
> -		fail = TREE_READONLY (elt);
> +		fail = TREE_READONLY (*elt);
>   		break;
>   	      }
>   	}
> @@ -5949,6 +5986,28 @@ cxx_eval_store_expression (const constex
>         valp = ctx->global->values.get (object);
>         for (unsigned i = 0; i < vec_safe_length (indexes); i++)
>   	{
> +	  ctors[i] = valp;
> +	  if (TREE_CODE (indexes[i]) == REALPART_EXPR
> +	      || TREE_CODE (indexes[i]) == IMAGPART_EXPR)
> +	    {
> +	      if (TREE_CODE (*valp) == COMPLEX_CST)
> +		*valp = build2 (COMPLEX_EXPR, TREE_TYPE (*valp),
> +				TREE_REALPART (*valp),
> +				TREE_IMAGPART (*valp));
> +	      else if (TREE_CODE (*valp) == CONSTRUCTOR
> +		       && CONSTRUCTOR_NELTS (*valp) == 0
> +		       && CONSTRUCTOR_NO_CLEARING (*valp))
> +		{
> +		  tree r = build_constructor (TREE_TYPE (TREE_TYPE (*valp)),
> +					      NULL);
> +		  CONSTRUCTOR_NO_CLEARING (r) = 1;
> +		  *valp = build2 (COMPLEX_EXPR, TREE_TYPE (*valp), r, r);
> +		}
> +	      gcc_assert (TREE_CODE (*valp) == COMPLEX_EXPR);
> +	      valp = &TREE_OPERAND (*valp,
> +				    TREE_CODE (indexes[i]) == IMAGPART_EXPR);
> +	      break;
> +	    }

Hmm, why do we need to handle complex in the !preeval case?  I'd think 
we want to preevaluate all complex values or components thereof.

>   	  constructor_elt *cep
>   	    = get_or_insert_ctor_field (*valp, indexes[i], index_pos_hints[i]);
>   	  valp = &cep->value;
> @@ -6012,17 +6071,41 @@ cxx_eval_store_expression (const constex
>     bool c = TREE_CONSTANT (init);
>     bool s = TREE_SIDE_EFFECTS (init);
>     if (!c || s || activated_union_member_p)
> -    for (tree elt : *ctors)
> +    for (tree *elt : ctors)
>         {
> +	if (TREE_CODE (*elt) != CONSTRUCTOR)
> +	  continue;
>   	if (!c)
> -	  TREE_CONSTANT (elt) = false;
> +	  TREE_CONSTANT (*elt) = false;
>   	if (s)
> -	  TREE_SIDE_EFFECTS (elt) = true;
> +	  TREE_SIDE_EFFECTS (*elt) = true;
>   	/* Clear CONSTRUCTOR_NO_CLEARING since we've activated a member of
>   	   this union.  */
> -	if (TREE_CODE (TREE_TYPE (elt)) == UNION_TYPE)
> -	  CONSTRUCTOR_NO_CLEARING (elt) = false;
> +	if (TREE_CODE (TREE_TYPE (*elt)) == UNION_TYPE)
> +	  CONSTRUCTOR_NO_CLEARING (*elt) = false;
>         }
> +  if (!indexes->is_empty ())
> +    {
> +      tree last = indexes->last ();
> +      if (TREE_CODE (last) == REALPART_EXPR
> +	  || TREE_CODE (last) == IMAGPART_EXPR)
> +	{
> +	  tree *cexpr = ctors.last ();
> +	  if (tree c = const_binop (COMPLEX_EXPR, TREE_TYPE (*cexpr),
> +				    TREE_OPERAND (*cexpr, 0),
> +				    TREE_OPERAND (*cexpr, 1)))
> +	    *cexpr = c;
> +	  else
> +	    {
> +	      TREE_CONSTANT (*cexpr)
> +		= (TREE_CONSTANT (TREE_OPERAND (*cexpr, 0))
> +		   & TREE_CONSTANT (TREE_OPERAND (*cexpr, 1)));
> +	      TREE_SIDE_EFFECTS (*cexpr)
> +		= (TREE_SIDE_EFFECTS (TREE_OPERAND (*cexpr, 0))
> +		   | TREE_SIDE_EFFECTS (TREE_OPERAND (*cexpr, 1)));

This seems like it needs to come before the ctors loop, so that these 
flags can be propagated out to enclosing constructors.

> +	    }
> +	}
> +    }
>   
>     if (lval)
>       return target;
> --- gcc/testsuite/g++.dg/cpp1y/constexpr-complex1.C.jj	2022-06-17 17:41:45.885780190 +0200
> +++ gcc/testsuite/g++.dg/cpp1y/constexpr-complex1.C	2022-06-17 17:41:45.885780190 +0200
> @@ -0,0 +1,24 @@
> +// PR c++/88174
> +// { dg-do compile { target c++14 } }
> +
> +constexpr bool
> +foo (double x, double y, double z, double w)
> +{
> +  __complex__ double a = 0;
> +  __real__ a = x;
> +  __imag__ a = y;
> +#if __cpp_constexpr >= 201907L
> +  __complex__ double b;
> +  __real__ b = z;
> +#else
> +  __complex__ double b = z;
> +#endif
> +  __imag__ b = w;
> +  a += b;
> +  a -= b;
> +  a *= b;
> +  a /= b;
> +  return __real__ a == x && __imag__ a == y;
> +}
> +
> +static_assert (foo (1.0, 2.0, 3.0, 4.0), "");
> 
> 
> 	Jakub
>
  

On Mon, Jun 20, 2022 at 04:03:50PM -0400, Jason Merrill wrote:
> > +      if (code == COMPLEX_TYPE)
> > +	{
> > +	  if (TREE_CODE (*valp) == COMPLEX_CST)
> > +	    *valp = build2 (COMPLEX_EXPR, type, TREE_REALPART (*valp),
> > +			    TREE_IMAGPART (*valp));
> > +	  else if (TREE_CODE (*valp) == CONSTRUCTOR
> > +		   && CONSTRUCTOR_NELTS (*valp) == 0
> > +		   && CONSTRUCTOR_NO_CLEARING (*valp))
> > +	    {
> > +	      tree r = build_constructor (reftype, NULL);
> > +	      CONSTRUCTOR_NO_CLEARING (r) = 1;
> > +	      *valp = build2 (COMPLEX_EXPR, type, r, r);
> > +	    }
> > +	  gcc_assert (TREE_CODE (*valp) == COMPLEX_EXPR);
> > +	  ctors.safe_push (valp);
> > +	  vec_safe_push (indexes, index);
> > +	  valp = &TREE_OPERAND (*valp, TREE_CODE (index) == IMAGPART_EXPR);
> > +	  gcc_checking_assert (refs->is_empty ());
> > +	  type = reftype;
> > +	  break;
> > +	}
...
> > @@ -5949,6 +5986,28 @@ cxx_eval_store_expression (const constex
> >         valp = ctx->global->values.get (object);
> >         for (unsigned i = 0; i < vec_safe_length (indexes); i++)
> >   	{
> > +	  ctors[i] = valp;
> > +	  if (TREE_CODE (indexes[i]) == REALPART_EXPR
> > +	      || TREE_CODE (indexes[i]) == IMAGPART_EXPR)
> > +	    {
> > +	      if (TREE_CODE (*valp) == COMPLEX_CST)
> > +		*valp = build2 (COMPLEX_EXPR, TREE_TYPE (*valp),
> > +				TREE_REALPART (*valp),
> > +				TREE_IMAGPART (*valp));
> > +	      else if (TREE_CODE (*valp) == CONSTRUCTOR
> > +		       && CONSTRUCTOR_NELTS (*valp) == 0
> > +		       && CONSTRUCTOR_NO_CLEARING (*valp))
> > +		{
> > +		  tree r = build_constructor (TREE_TYPE (TREE_TYPE (*valp)),
> > +					      NULL);
> > +		  CONSTRUCTOR_NO_CLEARING (r) = 1;
> > +		  *valp = build2 (COMPLEX_EXPR, TREE_TYPE (*valp), r, r);
> > +		}
> > +	      gcc_assert (TREE_CODE (*valp) == COMPLEX_EXPR);
> > +	      valp = &TREE_OPERAND (*valp,
> > +				    TREE_CODE (indexes[i]) == IMAGPART_EXPR);
> > +	      break;
> > +	    }
> 
> Hmm, why do we need to handle complex in the !preeval case?  I'd think we
> want to preevaluate all complex values or components thereof.

Because the late evaluation of the initializer could have touched
the destination, so we need to reevaluate it.
Same reason why we call get_or_insert_ctor_field again in the second
loop as we call it in the first loop.
If it would help, I could move that repeated part into:
tree
canonicalize_complex_to_complex_expr (tree t)
{
  if (TREE_CODE (t) == COMPLEX_CST)
   t = build2 (COMPLEX_EXPR, TREE_TYPE (t),
	       TREE_REALPART (t), TREE_IMAGPART (t));
  else if (TREE_CODE (t) == CONSTRUCTOR
	   && CONSTRUCTOR_NELTS (t) == 0
	   && CONSTRUCTOR_NO_CLEARING (t))
    {
      tree r = build_constructor (TREE_TYPE (TREE_TYPE (t)), NULL);
      CONSTRUCTOR_NO_CLEARING (r) = 1;
      t = build2 (COMPLEX_EXPR, TREE_TYPE (t), r, r);
    }
  return t;
}
and use that to shorten the code.

> 
> >   	  constructor_elt *cep
> >   	    = get_or_insert_ctor_field (*valp, indexes[i], index_pos_hints[i]);
> >   	  valp = &cep->value;
> > @@ -6012,17 +6071,41 @@ cxx_eval_store_expression (const constex
> >     bool c = TREE_CONSTANT (init);
> >     bool s = TREE_SIDE_EFFECTS (init);
> >     if (!c || s || activated_union_member_p)
> > -    for (tree elt : *ctors)
> > +    for (tree *elt : ctors)
> >         {
> > +	if (TREE_CODE (*elt) != CONSTRUCTOR)
> > +	  continue;
> >   	if (!c)
> > -	  TREE_CONSTANT (elt) = false;
> > +	  TREE_CONSTANT (*elt) = false;
> >   	if (s)
> > -	  TREE_SIDE_EFFECTS (elt) = true;
> > +	  TREE_SIDE_EFFECTS (*elt) = true;
> >   	/* Clear CONSTRUCTOR_NO_CLEARING since we've activated a member of
> >   	   this union.  */
> > -	if (TREE_CODE (TREE_TYPE (elt)) == UNION_TYPE)
> > -	  CONSTRUCTOR_NO_CLEARING (elt) = false;
> > +	if (TREE_CODE (TREE_TYPE (*elt)) == UNION_TYPE)
> > +	  CONSTRUCTOR_NO_CLEARING (*elt) = false;
> >         }
> > +  if (!indexes->is_empty ())
> > +    {
> > +      tree last = indexes->last ();
> > +      if (TREE_CODE (last) == REALPART_EXPR
> > +	  || TREE_CODE (last) == IMAGPART_EXPR)
> > +	{
> > +	  tree *cexpr = ctors.last ();
> > +	  if (tree c = const_binop (COMPLEX_EXPR, TREE_TYPE (*cexpr),
> > +				    TREE_OPERAND (*cexpr, 0),
> > +				    TREE_OPERAND (*cexpr, 1)))
> > +	    *cexpr = c;
> > +	  else
> > +	    {
> > +	      TREE_CONSTANT (*cexpr)
> > +		= (TREE_CONSTANT (TREE_OPERAND (*cexpr, 0))
> > +		   & TREE_CONSTANT (TREE_OPERAND (*cexpr, 1)));
> > +	      TREE_SIDE_EFFECTS (*cexpr)
> > +		= (TREE_SIDE_EFFECTS (TREE_OPERAND (*cexpr, 0))
> > +		   | TREE_SIDE_EFFECTS (TREE_OPERAND (*cexpr, 1)));
> 
> This seems like it needs to come before the ctors loop, so that these flags
> can be propagated out to enclosing constructors.

I could indeed move this in between
  bool c = TREE_CONSTANT (init);
  bool s = TREE_SIDE_EFFECTS (init);
and
  if (!c || s || activated_union_member_p)
and update c and s from *cexpr flags.

	Jakub