Segher Boessenkool <seg...@kernel.crashing.org> writes:
> Hi!
>
> On Mon, Sep 07, 2020 at 09:20:59PM +0100, Richard Sandiford wrote:
>> This is just personal opinion, but in general (from the point of view
>> of a new port, or a new subport like SVE), I think the best approach
>> to handling the "type" attribute is to start with the coarsest
>> classification that makes sense, then split these relatively coarse
>> types up whenever there's a specific need.
>
> Agreed.
>
>> When taking that approach, it's OK (and perhaps even a good sign)
>> for an existing type to sometimes be too coarse for a new CPU.
>>
>> So thanks for asking about this, and please don't feel constrained
>> by the existing "type" classification. IMO we should split existing
>> types wherever that makes sense for new CPUs.
>
> You can also use some other attributes to classify instructions, you
> don't have to put it all in one "type" attribute. This can of course be
> done later, at a time when it is clearer what a good design will be.
> Sometimes it is obvious from the start though :-)
>
> (This primarily makes the pipeline descriptions much simpler, but also
> custom scheduling code and the like. If one core has two types of "A"
> insn, say "Aa" and "Ab", it isn't nice if all other cores now have to
> handle both "Aa" and "Ab" instead of just "A").
I guess it makes the description of other cores more verbose, but it
doesn't IMO make them more complicated. It's still just one test
against one attribute. And updating existing descriptions can be
done automatically by sed.
The difficulty with splitting into subattributes is that the tests in
the cores that *do* care then become more complicated. E.g. you have
to do:
(ior (and (eq_attr "type" "foo")
(eq_attr "foo_subtype" "foo1"))
(eq_attr "type" "...others.."))
and:
(ior (and (eq_attr "type" "foo")
(eq_attr "foo_subtype" "!foo1"))
(eq_attr "type" "...others.."))
instead of just:
(eq_attr "type" "...")
in both cases.
It's not too bad when it's just one subtype (like above), but it could
easily get out of hand.
Perhaps in this case there's an argument in favour of having a separate
attribute due to combinatorial explosion. E.g. we have:
- alu_shift_imm
- alus_shift_imm
- logic_shift_imm
- logics_shift_imm
so having one attribute that describes the shift in all four cases
would perhaps be better than splitting each of them into two.
Arguments in the other direction:
- Once we have a separate attribute, it isn't clear where the line
should be drawn. Alternatives include:
(1) keeping the new attribute specific to shift immediates only
(2) also having a “register” value, and folding:
alu_shift_imm, alu_shift_reg -> alu_shift
(3) also having a “none” value, and doing away with the *_shift
type variants altogether:
alu_sreg, alu_shift_imm, alu_shift_reg -> alu_reg
I think we should have a clear reason for whichever we pick,
otherwise we could be creating technical debt for later.
- That approach is the opposite of what we did for the neon_* attributes:
every type has a q variant, rather than the size being a separate
attribute.
FWIW, we shouldn't assume that this distinction is specific to a64fx. :-)
Thanks,
Richard
