On Fri, 4 Jun 2021, Jakub Jelinek wrote:
> On Fri, Jun 04, 2021 at 04:06:43PM +0200, Richard Biener wrote:
> > On June 4, 2021 10:44:42 AM GMT+02:00, Jakub Jelinek <ja...@redhat.com>
> > wrote:
> > >Hi!
> > >
> > >The callers of fold_read_from_vector expect that the index they pass is
> > >an index of an element in the vector and the function does that most of
> > >the
> > >time. But we allow CONSTRUCTORs with VECTOR_TYPE to have VECTOR_TYPE
> > >elements and in that case every CONSTRUCTOR element represents not just
> > >one
> > >index (with the exception of V1 vectors), but multiple.
> > >So returning zero vector if i >= CONSTRUCTOR_NELTS or returning some
> > >CONSTRUCTOR_ELT's value might not be what the callers expect.
> > >
> > >Fixed by punting if the first element has vector type.
> > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > >
> > >In theory we could instead recurse (and assert that for CONSTRUCTORs of
> > >vector elements we have always all elements specified like tree-cfg.c
> > >verifies?) after adjusting the index appropriately.
> >
> > I think we do this in the corresponding BIT_FIELD_REF match.pd rule.
>
> The fold_read_from_vector is indeed only called from the BIT_FIELD_REF
> match.pd rule (3 times there), but I don't see there any number of elements
> checking or this vector elt checking.
>
> Do you want me to do this check in match.pd instead of
> fold_read_from_vector?
Hmm, since fold_read_from_vector is stand-alone we should make it work
for any CONSTRUCTOR so the guard there is OK. The match.pd rule using
it is the vec_perm one - what I was remembering is that we have
/* Simplify vector extracts. */
(simplify
(BIT_FIELD_REF CONSTRUCTOR@0 @1 @2)
...
/* Constructor elements can be subvectors. */
poly_uint64 k = 1;
...
so I suppose implementing fold_read_from_vector in terms of simplifying
the corresponding bit-field-ref should "work".
But then I think your original patch is OK.
Thanks,
Richard.
> > Note I don't think we allow CONSTRUCTOR (as in GENERIC) elements, so you'd
> > only see SSA names with vector type here?
>
> The match.pd rule is both GIMPLE and GENERIC, so I don't see why
> it couldn't be there a VECTOR_TYPE CONSTRUCTOR with CONSTRUCTOR elements
> (or e.g. one VAR_DECL element and one CONSTRUCTOR element).
> In particular, on the testcase I see in *.original
> W D.2844 = { 0, 0, 0, 0, 0, 0, 0, 0 };
>
> return VIEW_CONVERT_EXPR<U>(BIT_FIELD_REF < VEC_PERM_EXPR < <<< Unknown
> tree: compound_literal_expr
> W D.2844 = { 0, 0, 0, 0, 0, 0, 0, 0 }; >>> , {v, { 0, 0, 0, 0 }} , { 0,
> 8, 2, 3, 4, 5, 6, 7 } > , 128, 0>);
> In this case fold_read_from_vector is called on the {v, { 0, 0, 0, 0 }}
> CONSTRUCTOR with 0 and so the recursion would be on VAR_DECL and would punt,
> but if the permutation was e.g.
> { 0, 13, 2, 3, 4, 5, 6, 7 }
> then it would be called with 5 as index and it could see that
> it is in the second half (aka. the { 0, 0, 0, 0 } constructor) and
> read the 5-4 element from there.
> Even in GIMPLE, I see up to before veclower21
> _1 = {v_3(D), { 0, 0, 0, 0 }};
> _2 = VEC_PERM_EXPR <{ 0, 0, 0, 0, 0, 0, 0, 0 }, _1, { 0, 8, 2, 3, 4, 5, 6,
> 7 }>;
> which seems like CONSTRUCTOR inside of CONSTRUCTOR.
> But sure, it could be an SSA_NAME too and if we wanted to recurse it
> would need to handle that case too and look at SSA_NAME_DEF_STMT.
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
