llvm dev - Oct 2019 - Adding MVT::i4 and MVT::i2

Hi,

as part of some work required in our RISC-V V-extension backend we would
like to have vector machine value types whose elements are of type i4 and
i2. Currently i4 and i2 are not MVT as such and are handled when needed as
extended value types (EVTs being, as far as I understand, like a superset
of MVTs).

One interesting effect of doing this is that I found that there may be
nodes with illegal types after type-legalization. One example (not sure if
there are others) is SIGN_EXTEND_INREG that has a special operand whose
type may be illegal (only MVTs can be legal, so any EVT that is not an MVT
I understand it must be considered illegal too). This operand represents
the "original" type (an EVT) being extended. Currently things work for
an
extension of i4 or i2 because operation legalization does the right thing
(i.e. Expand) when it finds an "extended" type (like i4 or i2). If the
type
is not extended it defers the action to the target.

By making these types MVT, now they are not extended types any more. By
default SIGN_EXTEND_INREG action is Legal so this causes instruction
selection problems (e.g. most backends choke when trying to sign extend
inreg from an i4 or an i2).

For now I have added the following to TargetLoweringBase

for (MVT VT : {MVT::i2, MVT::i4}) {
   setOperationAction(ISD::SIGN_EXTEND_INREG, VT, Expand);
}

which avoids most of the crashes though there are a few CodeGen tests that
are
changing. These are AArch64/fast-isel-branch-cond-mask.ll, AMDGPU/idot8u.ll
and
X86/load-local-v3i1.ll.

I presume the change should be in principle NFC so I'm left wondering if
this is the
right approach. Maybe it is not because there are underlying assumptions or
implications I'm unaware for MVTs smaller than i8 (currently only i1).

Is introducing sub-i8 MVTs reasonable?

Thank you very much,

-- 
Roger Ferrer Ibáñez
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20191007/495b5d76/attachment.html>

> On Oct 7, 2019, at 3:50 AM, Roger Ferrer Ibáñez via llvm-dev <llvm-dev
at lists.llvm.org> wrote:
> 
> Hi,
> 
> as part of some work required in our RISC-V V-extension backend we would
like to have vector machine value types whose elements are of type i4 and i2.
Currently i4 and i2 are not MVT as such and are handled when needed as extended
value types (EVTs being, as far as I understand, like a superset of MVTs).
> 
> One interesting effect of doing this is that I found that there may be
nodes with illegal types after type-legalization. One example (not sure if there
are others) is SIGN_EXTEND_INREG that has a special operand whose type may be
illegal (only MVTs can be legal, so any EVT that is not an MVT I understand it
must be considered illegal too). This operand represents the
"original" type (an EVT) being extended. Currently things work for an
extension of i4 or i2 because operation legalization does the right thing (i.e.
Expand) when it finds an "extended" type (like i4 or i2). If the type
is not extended it defers the action to the target.
> 
> By making these types MVT, now they are not extended types any more. By
default SIGN_EXTEND_INREG action is Legal so this causes instruction selection
problems (e.g. most backends choke when trying to sign extend inreg from an i4
or an i2).
> 
> For now I have added the following to TargetLoweringBase
> 
> for (MVT VT : {MVT::i2, MVT::i4}) {
>    setOperationAction(ISD::SIGN_EXTEND_INREG, VT, Expand);
> }
> 
> which avoids most of the crashes though there are a few CodeGen tests that
are
> changing. These are AArch64/fast-isel-branch-cond-mask.ll, AMDGPU/idot8u.ll
and
> X86/load-local-v3i1.ll.
> 
> I presume the change should be in principle NFC so I'm left wondering
if this is the
> right approach. Maybe it is not because there are underlying assumptions or
implications I'm unaware for MVTs smaller than i8 (currently only i1).
> 
> Is introducing sub-i8 MVTs reasonable?
The general design intention of MVTs is that we should add them when a supported
target needs them, so I think this is the right approach.  Are you proposing
that all the vectors of i2/i4 be supported as well as MVTs?

The issues you’re pointing out above are long standardizing design problems - I
think that adding those actions to TargetLoweringBase is totally reasonable.  Do
you know what the root of the other changed codegen tests are?

-Chris