Re: A problem with one instruction multiple latencies and pipelines

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Fri, Sep 11, 2020 at 08:44:54AM +0100, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > 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.

Combinatorial explosion is real.

(ior)s etc. are easier to read, and importantly it is much more obvious
if any case is missed with them.

Here is the very first diff in d839f53b7dfd, where I started this for
rs6000:

 (define_insn_reservation "ppc403-load" 2
-  (and (eq_attr "type" "load,load_ext,load_ext_u,load_ext_ux,load_ux,load_u,\
-                       load_l,store_c,sync")
+  (and (eq_attr "type" "load,load_l,store_c,sync")
        (eq_attr "cpu" "ppc403,ppc405"))
   "iu_40x")

We started off with 77 insn types.  Now we have 63 (it was 57 before, we
added some more).

> > 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.

No, it very much had to, because the existing code missed many cases and
was inconsistent in others.

> 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.

It is very much not what I am saying.  I *am* saying that if people
trying to improve one CPU's modelling have to edit over 20 models for
CPUs that they do not care about, mistakes happen.  Such churn is
conceptually wrong as well, of course (the new types are just the same
as the old types on most older CPUs!)

> > 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.

Not in this case; all such insns always have it as op 2 in the machine
instruction, and RTL almost always uses the same order.

> Anyway, I think we're in violent agreement on how this works, we're just
> taking different conclusions from it.

Yeah :-)  And not even so different: I think you agree it is a
trade-off, you just see it tilt more to one side, while I see it go to
the other side often.

We still have over fifty instruction types.  We (hopefully :-) ) use the
"extra attribute" method only where that helps more than it hurts.  It
is just another tool in the toolbox.


Segher