Segher Boessenkool <seg...@kernel.crashing.org> writes:
> On Thu, Sep 10, 2020 at 11:04:26AM +0100, Richard Sandiford wrote:
>> Segher Boessenkool <seg...@kernel.crashing.org> writes:
>> > 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.
>
> Consider cores that do not care about the "subtype" at all: when using
> just "type", all cores have to test for "foo,foo_subtype", while with
> a separate attribute they can just test for "foo".
Right. But that was exactly the case I was considering with the sed
comment above :-)
I don't see adding a new attribute value to a set_attr as extra
complication. It's just adding a value to a set. To me extra
complication is adding (ior …)s, (and …)s, etc.
> A typical example in rs6000 is the "sign_extend" attribute for load
> instructions. All cores before power4 do not care at all about it.
> (Load and store insns get into combinatorial explosion land as well,
> as you discuss below, with "update" and "indexed" forms).
>
>> 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.
>
> Yes. It is a trade-off.
>
>> 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.
>
> Yeas, exactly. And for rs6000 we *did* have many more types before, and
> very frequently one was missed (in a scheduling description usually).
> Especially common was when some new type attribute was added, not all
> existing cpu descriptions were updated correctly. Maybe this is the
> strongest argument "for" actually :-)
The update shouldn't be done by hand. The regularity of the syntax
means that an appropriate sed really is good enough (perhaps with
formatting fixes on top). For extra robustness, you can replace the
old attribute value with two new ones, so that any missed case triggers
a build error.
What I'm really wary of with taking the line above is that it feeds
into the attitude that existing scheduling descriptions should become
fossilised at the point they're added: noone working on a different
core should change the declarations in the file later in case they
miss something. I don't think that's what you're saying, but it
could feed into that general attitude.
>> Arguments in the other direction:
>>
>> - Once we have a separate attribute, it isn't clear where the line
>> should be drawn.
>
> Good taste ;-)
>
>> 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.
>
> Yes.
>
> One example of what we do:
>
> ;; Is this instruction using operands[2] as shift amount, and can that be a
> ;; register?
> ;; This is used for shift insns.
> (define_attr "maybe_var_shift" "no,yes" (const_string "no"))
>
> ;; Is this instruction using a shift amount from a register?
> ;; This is used for shift insns.
> (define_attr "var_shift" "no,yes"
> (if_then_else (and (eq_attr "type" "shift")
> (eq_attr "maybe_var_shift" "yes"))
> (if_then_else (match_operand 2 "gpc_reg_operand")
> (const_string "yes")
> (const_string "no"))
> (const_string "no")))
>
> define_insns only use maybe_var_shift. Only the power6 and e*500* cpu
> descriptions ever look at var_shift. (Directly. There is some other
> scheduling code that looks at it too -- and all but the power6 ones seem
> to be incorrect! That is all a direct translation of "type-only" code
> there was before...)
Right. This is similar to the:
(define_attr "shift_imm_type" "none,alu_shift_op2")
thing I suggested upthread. I think it's better for the define_insn
attribute to specify the operand number directly, since there might
well be cases where the shift isn't operand 2.
Anyway, I think we're in violent agreement on how this works, we're just
taking different conclusions from it.
Thanks,
Richard
