On Thu, Feb 6, 2020 at 7:02 AM Jakub Jelinek <ja...@redhat.com> 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).
>
Where are these verifiers? I'm not finding them.
> 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?
>
That sounds right to me. Until we have the linear range start marker you
suggested above.
> 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?
>
Yes, though only for omitted initializers where {}-initialization is
different from zero-initialization; we don't currently combine explicit
initializers.
> I guess right now we emit indexes for all elements for those, but if we
> choose to optimize?
>
> > 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.
>
If we go back and fill in indexes when we see something more complicated,
we could enforce this in the verifiers.
> > > + 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);
>
Let's change this assert to allow null index.
>
> 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
>
>
