[v2] c++: Handle CONSTRUCTORs without indexes in find_array_ctor_elt [PR93549]

Hi!

On Thu, Feb 06, 2020 at 05:04:49PM +0100, Jakub Jelinek wrote:
> On Thu, Feb 06, 2020 at 10:38:25AM -0500, Jason Merrill wrote:
> > > I don't know, can try to add some instrumentation and do bootstrap/regtest
> > > with it.  The handling of the CONSTRUCTORs with missing or present or mixed
> > > indexes is what I found in various middle-end routines.
> > > The only thing I see in our verifiers is that in GIMPLE function bodies,
> > > we don't allow non-VECTOR_TYPE CONSTRUCTORs with any elements, and for
> > > VECTOR_TYPE CONSTRUCTORs we require that indexes are NULL for elements with
> > > VECTOR_TYPE and for others require that it is either NULL or INTEGER_CST
> > > matching the position (so effectively for those direct access is still
> > > possible).
> > >
> > 
> > Where are these verifiers?  I'm not finding them.
> 
> tree-cfg.c (verify_gimple_assign_single).
> Though, we don't really have verifiers for initializers of global variables,
> guess it would need to be called somewhere from varpool_node::assemble_decl
> or so (or other varpool method or multiple of them).

Here is a new version, which assumes CONSTRUCTORs with all or none indexes
and for CONSTRUCTORs without indexes performs the verification for
flag_checking directly in find_array_ctor_elt.  For CONSTRUCTORs with
indexes, it doesn't do the verification of all elts, because some CONSTRUCTORs
can be large, and it "verifies" only what it really needs - if all elts
touched during the binary search have indexes, that is actually all we care
about because we are sure we found the right elt.  It is just if we see a
missing index we need assurance that all are missing to be able to directly
access it.

The assumption then simplifies the patch, for no index CONSTRUCTORs we can
use direct access like for CONSTRUCTORs where last elt index is equal to the
elt position.  If we append right after the last elt, we just should clear
the index so that we don't violate the assumption, and if we need a gap
between the elts and the elt to be added, we need to add indexes.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2020-02-08  Jakub Jelinek  <jakub@redhat.com>

	PR c++/93549
	* constexpr.c (find_array_ctor_elt): If last element has no index,
	for flag_checking verify all elts have no index.  If i is within the
	elts, return it directly, if it is right after the last elt, append
	if NULL index, otherwise force indexes on all elts.
	(cxx_eval_store_expression): Allow cep->index to be NULL.

	* g++.dg/ext/constexpr-pr93549.C: New test.



	Jakub

On 2/8/20 5:14 AM, Jakub Jelinek wrote:
> Hi!
> 
> On Thu, Feb 06, 2020 at 05:04:49PM +0100, Jakub Jelinek wrote:
>> On Thu, Feb 06, 2020 at 10:38:25AM -0500, Jason Merrill wrote:
>>>> I don't know, can try to add some instrumentation and do bootstrap/regtest
>>>> with it.  The handling of the CONSTRUCTORs with missing or present or mixed
>>>> indexes is what I found in various middle-end routines.
>>>> The only thing I see in our verifiers is that in GIMPLE function bodies,
>>>> we don't allow non-VECTOR_TYPE CONSTRUCTORs with any elements, and for
>>>> VECTOR_TYPE CONSTRUCTORs we require that indexes are NULL for elements with
>>>> VECTOR_TYPE and for others require that it is either NULL or INTEGER_CST
>>>> matching the position (so effectively for those direct access is still
>>>> possible).
>>>>
>>>
>>> Where are these verifiers?  I'm not finding them.
>>
>> tree-cfg.c (verify_gimple_assign_single).
>> Though, we don't really have verifiers for initializers of global variables,
>> guess it would need to be called somewhere from varpool_node::assemble_decl
>> or so (or other varpool method or multiple of them).
> 
> Here is a new version, which assumes CONSTRUCTORs with all or none indexes
> and for CONSTRUCTORs without indexes performs the verification for
> flag_checking directly in find_array_ctor_elt.  For CONSTRUCTORs with
> indexes, it doesn't do the verification of all elts, because some CONSTRUCTORs
> can be large, and it "verifies" only what it really needs - if all elts
> touched during the binary search have indexes, that is actually all we care
> about because we are sure we found the right elt.  It is just if we see a
> missing index we need assurance that all are missing to be able to directly
> access it.
> 
> The assumption then simplifies the patch, for no index CONSTRUCTORs we can
> use direct access like for CONSTRUCTORs where last elt index is equal to the
> elt position.  If we append right after the last elt, we just should clear
> the index so that we don't violate the assumption, and if we need a gap
> between the elts and the elt to be added, we need to add indexes.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Looks good, thanks.

> 2020-02-08  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/93549
> 	* constexpr.c (find_array_ctor_elt): If last element has no index,
> 	for flag_checking verify all elts have no index.  If i is within the
> 	elts, return it directly, if it is right after the last elt, append
> 	if NULL index, otherwise force indexes on all elts.
> 	(cxx_eval_store_expression): Allow cep->index to be NULL.
> 
> 	* g++.dg/ext/constexpr-pr93549.C: New test.
> 
> --- gcc/cp/constexpr.c.jj	2020-02-05 11:12:33.632383924 +0100
> +++ gcc/cp/constexpr.c	2020-02-07 19:30:10.549836834 +0100
> @@ -3028,8 +3028,32 @@ find_array_ctor_elt (tree ary, tree dind
>     if (end > 0)
>       {
>         tree cindex = (*elts)[end - 1].index;
> -      if (TREE_CODE (cindex) == INTEGER_CST
> -	  && compare_tree_int (cindex, end - 1) == 0)
> +      if (cindex == NULL_TREE)
> +	{
> +	  /* Verify that if the last index is missing, all indexes
> +	     are missing.  */
> +	  if (flag_checking)
> +	    for (unsigned int j = 0; j < len - 1; ++j)
> +	      gcc_assert ((*elts)[j].index == NULL_TREE);
> +	  if (i < end)
> +	    return i;
> +	  else
> +	    {
> +	      begin = end;
> +	      if (i == end)
> +		/* If the element is to be added right at the end,
> +		   make sure it is added with cleared index too.  */
> +		dindex = NULL_TREE;
> +	      else if (insert)
> +		/* Otherwise, in order not to break the assumption
> +		   that CONSTRUCTOR either has all indexes or none,
> +		   we need to add indexes to all elements.  */
> +		for (unsigned int j = 0; j < len; ++j)
> +		  (*elts)[j].index = build_int_cst (TREE_TYPE (dindex), j);
> +	    }
> +	}
> +      else if (TREE_CODE (cindex) == INTEGER_CST
> +	       && compare_tree_int (cindex, end - 1) == 0)
>   	{
>   	  if (i < end)
>   	    return i;
> @@ -4551,7 +4575,8 @@ cxx_eval_store_expression (const constex
>   	    = find_array_ctor_elt (*valp, index, /*insert*/true);
>   	  gcc_assert (i >= 0);
>   	  cep = CONSTRUCTOR_ELT (*valp, i);
> -	  gcc_assert (TREE_CODE (cep->index) != RANGE_EXPR);
> +	  gcc_assert (cep->index == NULL_TREE
> +		      || TREE_CODE (cep->index) != RANGE_EXPR);
>   	}
>         else
>   	{
> --- gcc/testsuite/g++.dg/ext/constexpr-pr93549.C.jj	2020-02-07 19:14:24.163815456 +0100
> +++ gcc/testsuite/g++.dg/ext/constexpr-pr93549.C	2020-02-07 19:14:24.163815456 +0100
> @@ -0,0 +1,26 @@
> +// PR c++/93549
> +// { dg-do compile { target c++17 } }
> +// { dg-options "-O2 -Wno-psabi -w" }
> +
> +struct simd {
> +  using shortx8 [[gnu::vector_size(16)]] = short;
> +  shortx8 data;
> +  constexpr simd (short x) : data{x, x, x, x, x, x, x, x} {}
> +  constexpr friend unsigned operator== (simd lhs, simd rhs)
> +  {
> +    shortx8 tmp = lhs.data == rhs.data;
> +    using ushort = unsigned short;
> +    auto bools = tmp ? ushort(1) : ushort(0);
> +    unsigned bits = 0;
> +    for (int i = 0; i < 8; ++i)
> +      bits |= bools[i] << i;
> +    return bits;
> +  }
> +};
> +
> +auto
> +foo ()
> +{
> +  constexpr auto tmp = simd(1) == simd(2);
> +  return tmp;
> +}
> 
> 
> 	Jakub
>