Re: RFC: Representing vector lane load/store operations

[Richard Guenther <richard.guenther@gmail.com>]

On Wed, Mar 23, 2011 at 1:18 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Guenther <richard.guenther@gmail.com> writes:
>> On Wed, Mar 23, 2011 at 11:38 AM, Richard Sandiford
>> <richard.sandiford@linaro.org> wrote:
>>> Richard Guenther <richard.guenther@gmail.com> writes:
>>>> But as you have partial defs of the vector lane array the simplest
>>>> approach is probably to not make them a register.  Be prepared
>>>> for some surprises during RTL expansion though ;)
>>>
>>> OK.  It's there I'd like to start, specifically with:
>>>
>>>  These arrays of vectors would still need to have a non-BLK mode,
>>>  so that they can be stored in _rtl_ registers.  But we need that anyway
>>>  for ARM's arm_neon.h; the code that today's GCC produces for the intrinsic
>>>  functions is very poor.
>>>
>>> because I'd like to fix the bad code we generate for intrinsics.
>>>
>>> Thing is, this is going to be another case where the mode of a type
>>> depends on the current target.  E.g. on ARM, we don't want to use
>>> a 24-byte mode for an array of 3 2xSI vectors unless V2SI is also
>>> available.  Both the mode of the vector type and the mode of the
>>> array type will therefore depend on whether Neon is enabled.
>>>
>>> I know you don't like the way we handle TYPE_MODE for vectors:
>>>
>>>  #define TYPE_MODE(NODE) \
>>>    (TREE_CODE (TYPE_CHECK (NODE)) == VECTOR_TYPE \
>>>     ? vector_type_mode (NODE) : (NODE)->type.mode)
>>>
>>> so I'm guessing you wouldn't be too happy to see ARRAY_TYPE popping
>>> up there as well. :-)  What's the best way of handling this?
>>
>> I'd say use either DECL_MODE at the point where we decide on
>> expanding vars (setting it from a target hook), or simply ask such
>> a hook at expansion time.  That should have worked for the target
>> atttribute stuff as well instead of dispatching in TYPE_MODE (types
>> are global and TYPE_MODE with the target attribute depends on
>> the context, but decls are local to the declaration context, so the
>> mode persists and is not dependent on the attribute). Might
>> need some surgery in places where we assume TYPE_MODE == DECL_MODE,
>> but I suspect it's mostly around RTL expansion.
>
> Hmm, but if we do that, when is it correct to look at TYPE_MODE?

Most of the tree passes shouldn't care about TYPE_MODE (nor
DECL_MODE) and on RTL we shouldn't need to care about trees.

> E.g. when expanding the new __builtin_load_lanes function described
> earlier, it wouldn't be valid to base the target register's mode on
> TYPE_MODE, so I suppose we'd have to call the hook instead.

Well, you'd expand __builtin_load_lanes only if the mode is available, no?
So you know the mode in advance and don't need to get it from anywhere.

>  And if we
> did revert the TYPE_MODE change for vector types, the vector optabs
> would need to do the same thing.  Wouldn't we just end up replacing
> most/all uses of TYPE_MODE with calls to the hook?  What would any
> remaining uses of TYPE_MODE actually be testing?

I think a lot of TYPE_MODE users are just lazy, like the optabs should
get a mode input and not use a type - the vectorizer knows what target
support it targets for so it can supply a proper mode.  Alternatively
extract the mode from the operands instead, using DECL_MODE.

That said, I think given that target support can change across functions
using something global like TYPE_MODE is fundamentally flawed
(unless you start doing ugly things like that callback in the TYPE_MODE
implementation).

> E.g. I suppose we really ought to do the same thing for 128-bit types,
> since this:
>
>    /* TODO: This isn't correct, but as logic depends at the moment on
>       host's instead of target's wide-integer.
>       If there is a target not supporting TImode, but has an 128-bit
>       integer-scalar register, this target check needs to be adjusted. */
>    if (targetm.scalar_mode_supported_p (TImode))
>      {
>        int128_integer_type_node = make_signed_type (128);
>        int128_unsigned_type_node = make_unsigned_type (128);
>      }
>
> seems to apply one value of scalar_mode_supported_p to the whole compilation.
> (TImode support seems to depend on TARGET_ZARCH for s390.)

Well, it depends on where int128_integer_type_node is used.  I think
if the target with some settings supports TImode then we probably
want to have that type node.  At the point the user declares some vars
with it you can error out dependent on local support.  At expansion
time you'd need to check whether accesses in a given mode are
really "possible" and dispatch to BLKmode handling if they are not.

The tree level really doesn't care, and most TYPE_MODE uses there
are bogus - the valid ones want to check targetm.xxxx_mode_supported_p
instead.  During RTL expansion we have to deal with handling modes
we don't support (or ICE, as we do now with a lot of target attribute
uses).

For your case in question the vectorizer would create local vars with
that mode, knowing it is supported, so I don't see big problems for
that particular case.

Richard.