Re: A problem with one instruction multiple latencies and pipelines

[Richard Sandiford <richard.sandiford@arm.com>]

"Qian, Jianhua" <qianjh@cn.fujitsu.com> writes:
> Hi Richard and Segher
>
> I don't know if I exactly understood your discussion.
> If I misunderstood, please let me know.
>
> I am trying to test these two cases.
> Case 1. keep the TYPE attribute unchanged, add new attributes
>     It works well as below.
>     (define_attr "shift_imm_value" "shift_imm_none,shift_imm_0,shift_imm_1to4,shift_imm_ge5" (const_string "shift_imm_none"))

I'm not sure the shift_imm_0 case is useful here FWIW.  alu_shift_imm
should only be used with nonzero shift amounts.

Also, the associated C++ enums are automatically prefixed with the name
of the attribute, so I think the values should just be 1to4, ge5, etc.,
with no prefix.

>     (define_insn "*add_<shift>_<mode>"
>       [(set (match_operand:GPI 0 "register_operand" "=r")
>             (plus:GPI (ASHIFT:GPI (match_operand:GPI 1 "register_operand" "r")
>                                   (match_operand:QI 2 "aarch64_shift_imm_<mode>" "n"))
>                       (match_operand:GPI 3 "register_operand" "r")))]
>       ""
>       "add\\t%<w>0, %<w>3, %<w>1, <shift> %2"
>       [(set_attr "type" "alu_shift_imm")
>        (set (attr "shift_imm_value")
>             (cond [(eq (symbol_ref "XINT(operands[2],0)") (const_int 0)) (const_string "shift_imm_0")
>                    (and (ge (symbol_ref "XINT(operands[2],0)") (const_int 1)) (le (symbol_ref "XINT(operands[2],0)") (const_int 4))) (const_string "shift_imm_1to4")]
>                    (const_string "shift_imm_ge5")))]
>     )

I'll come back to this below.

>
>     (define_insn_reservation "a64fx_alu_shift1to4" 2
>       (and (eq_attr "tune" "a64fx")
>            (eq_attr "type" "alu_shift_imm")
>            (eq_attr "shift_imm_value" "shift_imm_1to4"))
>       "(a64fx_exa,a64fx_exa)|(a64fx_exb,a64fx_exb)")
>
>
> Case2. expand the TYPE attribute
>     I replaced the alu_shift_imm to alu_shift_imm, alu_shift_imm1to4, and I got the error message " bad number of entries in SET_ATTR_ALTERNATIVE, was 2 expected 1"
>     (define_attr "type"
>       ...
>       alu_shift_imm,\
>       alu_shift_imm1to4,\
>       ...
>
>     (define_insn "*add_<shift>_<mode>"
>       [(set (match_operand:GPI 0 "register_operand" "=r")
>             (plus:GPI (ASHIFT:GPI (match_operand:GPI 1 "register_operand" "r")
>                                   (match_operand:QI 2 "aarch64_shift_imm_<mode>" "n"))
>                       (match_operand:GPI 3 "register_operand" "r")))]
>       ""
>       "add\\t%<w>0, %<w>3, %<w>1, <shift> %2"
>       [(set_attr "type" "alu_shift_imm,alu_shift_imm1to4")]
>     )
>
>     It means that one "type" value should be matched by one operand constraint pattern. So this will raise two problems.
>       1. One instruction should be classified to multiple patterns
>          - define multiple operand constraints, such as "(match_operand:QI 2 "aarch64_shift_imm_<mode>" "0, 1to4, ...")"
>          - or, use "cond" at set_attr just like Case1.
>       2. Whatever you do with the first problem, the type attribute cannot be set with "alu_shift_imm"
>         and "alu_shift_imm1to4" at the same time by one operand constraint pattern.

Right.  Something needs to tell the compiler how to distinguish between
the two cases.

>             - If we cannot resolve it, the existing CPUs' descriptions need to be changed. This is not what I expected.
>             - If we want to add new attribute to resolve this problem, why not use the Case1 directly?

It's a trade-off.  At the moment the rule for arm and aarch64 is simple:
the "type" attribute specifies the complete scheduling type.  If we
instead change policy and divide the information among several attributes,
those attributes will have increasingly complex relationships with one
another.  It will become easier to specify a meaningless combination
by mistake, or to forget to specify one of the relevant attributes.

I agree it's not so bad with just one extra attribute.  The question
is more: if we change the policy, what would things look like if there
were more attributes of a similar nature?

On the other side, lumping everything into one attributes gives a
long list of values.  I think the difference of opinion is that Segher
finds this argument more compelling while I find the other argument
more compelling ;-)

That said, there are really two issues here:

(1) how should the define_insn describe the instruction?
(2) how should scheduling descriptions test for it?

The same answer for (1) can support multiple answers for (2).

For (1), I think we should do what I mentioned in my first reply
and add something like (different name from last time):

  ;; config/arm/types.md
  (define_attr "autodetect_type" "none,alu_shift_op2")

This attribute would in principle be a home for any case in which
the scheduling type needs to be derived from other information
(in this case using the contents of operand 2 to infer the alu_shift
kind).  So going back to what I said above: we would be changing policy
so that define_insns can use one of *two* attributes to specify the
scheduling type:

- they can use "type" to specify an exact scheduling type directly
- they can use "autodetect_type" to specify a method for deriving the
  scheduling type from other information

The *add_<shift>_<mode> define_insn above would then use:

  [(set_attr "autodetect_type" "alu_shift_op2")]

The advantage of this IMO is that we still only have two attributes,
even if we need to autodetect other cases in future.  It also means
that there is only one copy of the condition to pick between (say)
1to4 and ge5.

This approach to (1) supports both approaches to (2).  If the
CPU models continue to use a single type attribute, we can use
autodetect_type to choose an appropriate default, using the method
I mentioned in my first reply:

  ;; config/arm/types.md
  (define_attr "type" "…long list…"
    (cond [(eq_attr "autodetect_type" "alu_shift_op2")
           (if_then_else (match_operand 2 "const_1_to_4_operand")
                         (const_string "alu_shift_imm_1to4")
                         (const_string "alu_shift_imm_ge5"))]
          (const_string "untyped")))

where:

- const_1_to_4_operand is a new predicate.  It could be defined in a
  new config/arm/common.md file that is shared by both arm and aarch64.

- alu_shift_imm is replaced by two new attributes: alu_shift_imm_1to4
  and alu_shift_imm_ge5.

(In principle, this condition could also handle alu_shift_reg,
if that proved to be useful later.)

If we instead use multiple attributes, the definitions would be:

(define_attr "type" "…"
  (cond [(eq_attr "autodetect_type" "alu_shift_op2")
         (const_string "alu_shift_imm")]
        (const_string "untyped")))

(define_attr "shift_imm_value" "none,1to4,ge5"
  (cond [(eq_attr "autodetect_type" "alu_shift_op2")
         (if_then_else (match_operand 2 "const_1_to_4_operand")
                       (const_string "1to4")
                       (const_string "ge5"))]
        (const_string "none"))

I still prefer the single type attribute, for the reasons above.

FWIW, this script updates the CPU descriptions so that every test
for alu_shift_imm is replaced by a test for the two new attributes:

find gcc/config/arm gcc/config/aarch64 -name '*.md' |
  while read arg; do
    case $arg in
      */arm.md | */aarch64.md | */types.md | */thumb1.md | */thumb2.md | \
      */arm-fixed.md)
	;;
      *)
	sed -i s/alu_shift_imm/alu_shift_imm_1to4,alu_shift_imm_ge5/g $arg
	;;
    esac
  done

(not idempotent :-))

Then you'd need to update the non-CPU files that were excluded by the
case statement above.

Although this looks/sounds complicated, the advantage is that everything
remains up-to-date.  If we instead added a second attribute and only
defined it for instructions like *add_<shift>_<mode>, other instructions
(including config/arm instructions) would still have type alu_shift_imm
but would have a shift_imm_value of "none".

Sorry for the long message.

Thanks,
Richard