On Thu, 6 Feb 2020, Jakub Jelinek wrote:
> On Wed, Feb 05, 2020 at 01:31:30PM -0500, Jason Merrill wrote:
> > > from the constexpr new change apparently broke the following testcase.
> > > When handling COND_EXPR, we build_vector_from_val, however as the
> > > argument we
> > > pass to it is not an INTEGER_CST/REAL_CST, but that wrapped in a
> > > NON_LVALUE_EXPR location wrapper, we end up with a CONSTRUCTOR and as it
> > > is
> > > middle-end that builds it, it doesn't bother with indexes. The
> > > cp_fully_fold_init call used to fold it into VECTOR_CST in the past, but
> > > as
> > > we intentionally don't invoke it anymore as it might fold away something
> > > that needs to be diagnosed during constexpr evaluation, we end up
> > > evaluating
> > > ARRAY_REF into the index-less CONSTRUCTOR. The following patch fixes the
> > > ICE by teaching find_array_ctor_elt to handle CONSTRUCTORs without indexes
> > > (that itself could be still very efficient) and CONSTRUCTORs with some
> > > indexes present and others missing (the rules are that if the index on the
> > > first element is missing, then it is the array's lowest index (in C/C++ 0)
> > > and if other indexes are missing, they are the index of the previous
> > > element
> > > + 1).
> >
> > Is it currently possible to get a CONSTRUCTOR with non-init-list type that
> > has some indexes present and others missing? Other than from the new code
> > in your patch that sets some indexes?
>
> 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).
> The question might not be just what we do emit right now, but also what we'd
> like to emit in the future, because as has been noted several times, for
> large initializers those explicit indexes consume huge amounts of memory.
> In C with designated initializers, I can see us not emitting indexes from
> the start because we'd want to avoid the memory overhead for normal
> sequential initializers, but then much later we can find a designated
> initializer that wants to skip over some elements and thus add an index at
> that point (or range designator for which we want RANGE_EXPR); shall we add
> indexes to all elements at that point?
> In C++, I think we don't allow non-useless array designated initializers, so
> there is no way to skip elements using that or go backwards, but still,
> don't we emit RANGE_EXPRs if we see the same initializer for many elements?
> I guess right now we emit indexes for all elements for those, but if we
> choose to optimize?
I've played with eliding them (on the C frontend) some time ago and
the issue with designated initializers is not themselves but that
we need to "sort" the CTOR and at that point we need indexes for all
elements (or have some other way of dealing with it).
Also for sparse CTORs the middle-end needs indices for binary search.
I wonder if we could replace INTEGER_CSTs with (index<<1)|1 in
constructor_elt.index or so ... (or sth less hackish)
I bet there's a way around the sorting issue of course. Like
the suggested late add.
> > Is it unreasonable to assume that if the first element has no index, none of
> > the elements do?
>
> Not sure, see above. Depends on what we want to guarantee.
In the middle-end we don't want to rely on this unless we have
checking that we honor it. For pure frontend code feel free to
set your own constraints ;)
> > > + else if (i == j + (middle - begin))
> > > + {
> > > + (*elts)[middle].index = dindex;
> >
> > Why set this index?
>
> Because the caller asserts or relies that it has one.
> constructor_elt *cep = NULL;
> if (code == ARRAY_TYPE)
> {
> HOST_WIDE_INT i
> = 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);
>
> Now, ATM we are aware of just small CONSTRUCTORs that can appear this way
> (VECTOR_TYPE and so generally not too many elements in real-world
> testcases), so if you prefer, the function when seeing NULL index could just
> add indexes to all elements and retry and defer deciding if and how we
> optimize large constructors for later.
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
