Hi,
Sorry for the slow reply, didn't see this till now.
Benoit Dinechin via Gcc <gcc@gcc.gnu.org> writes:
> Hello,
>
> I use (define_subst ...) in a way that solves my .md factoring
> problems but I wonder if this use is expected and if will be
> maintained / documented in the future.
>
> Target is a 64-bit core whose GPRs are 64-bit wide, but it also has
> 32-bit integer arithmetic instructions that clear the 32 upper bits of
> the destination register. For each instruction template,
> e.g. "addsi3", I define another one that zero-extends the result to 64
> bits from 32 bits. Before using (define_subst ...), that was:
>
> (define_insn "addsi3"
> [(set (match_operand:SI 0 "register_operand" "=r,r,r")
> (plus:SI (match_operand:SI 1 "register_operand" "r,r,r")
> (match_operand:SI 2 "register_w32_operand" "r,I10,W32")))]
> ""
> "addw %0 = %1, %2"
> [(set_attr "type" "alu_tiny,alu_tiny_w,alu_tiny_x")
> (set_attr "length" "4,4,8")]
> )
> ;; zero-extend version of addsi3
> (define_insn "*addsi3_zext"
> [(set (match_operand:DI 0 "register_operand" "=r,r,r")
> (zero_extend:DI (plus:SI (match_operand:SI 1 "register_operand"
> "r,r,r")
> (match_operand:SI 2 "register_w32_operand"
> "r,I10,W32")))]
> ""
> "addw %0 = %1, %2"
> [(set_attr "type" "alu_tiny,alu_tiny_w,alu_tiny_x")
> (set_attr "length" "4,4,8")]
> )
>
> The issue is to transform (set (match_operand:SI 0 ...)) to
> (set (match_operand:DI 0 ...)), that is, change SI mode to DI mode.
> However, using (match_dup 0) in the (define_subst ...) output-template
> expands to (set (match_operand:SI 0 ...)), that is, not DI mode. So I
> wrote the (define_subst ...) to force DI mode as follows:
>
> (define_subst "arith_zx_subst"
> [(set (match_operand:SI 0 "" "")
> (match_operand:SI 1 "" ""))]
> ""
> [(set (match_operand:DI 0 "register_operand" "=r")
> (zero_extend:DI (match_dup 1)))]
> )
>
> (define_insn "addsi3<arith_zx>"
> [(set (match_operand:SI 0 "register_operand" "=r,r,r")
> (plus:SI (match_operand:SI 1 "register_operand" "r,r,r")
> (match_operand:SI 2 "register_w32_operand" "r,I10,W32")))]
> ""
> "addw %0 = %1, %2"
> [(set_attr "type" "alu_tiny,alu_tiny_w,alu_tiny_x")
> (set_attr "length" "4,4,8")]
> )
>
> That is, the output-template of (define_subst ...) uses (match_operand ...)
> to force the DI mode, and does not use (match_dup ..) as
> explained in the Internals. Is that the rigth way to do it?
Yeah, that looks right. The downside is that the "=r" overwrites whatever
the original pattern used, so something like:
(match_operand:SI 0 "register_operand" "=r,f,f")
would (IIRC) get silently converted to:
(match_operand:DI 0 "register_operand" "=r,r,r")
But the define_subst is safe if the destination is always an "r".
> Then, by looking at the mddump, I noticed a seemingly strange value
> for the attribute generated by the (define_subst ...) expansion:
>
> ;; ../../gcc/config/kvx/scalar.md: 1044
> (define_insn ("addsi3")
> [
> (set (match_operand:SI 0 ("register_operand") ("=r,r,r"))
> (plus:SI (match_operand:SI 1 ("register_operand") ("r,r,r"))
> (match_operand:SI 2 ("register_w32_operand")
> ("r,I10,W32"))))
> ] ("") ("addw %0 = %1, %2")
> [
> (set_attr ("type") ("alu_tiny,alu_tiny_w,alu_tiny_x"))
> (set_attr ("length") ("4,4,8"))
> ])
>
> ;; ../../gcc/config/kvx/scalar.md: 1044
> (define_insn ("addsi3_zx")
> [
> (set (match_operand:DI 0 ("register_operand") ("=r,r,r"))
> (zero_extend:DI (plus:SI (match_operand:SI 1
> ("register_operand") ("r,r,r"))
> (match_operand:SI 2 ("register_w32_operand")
> ("r,I10,W32")))))
> ] ("") ("addw %0 = %1, %2")
> [
> (set_attr ("type") ("alu_tiny,alu_tiny_w,alu_tiny_x"))
> (set_attr ("length") ("4,4,8"))
> (set_attr ("arith_zx_subst") ("no"))
> ])
>
> Precisely, attribute "arith_zx_subst" is set to "no", whereas it has
> been declared by GCC in the mddump as:
>
> ;; ../../gcc/config/kvx/scalar.md: 1035
> (define_attr ("arith_zx_subst") ("no,yes")
> (const_string ("no")))
>
> Is there a way for "arith_zx_subst" to have the "yes" value in the
> templates generated by (define_subst ...)?
No, not out of the box. These attributes are used for internal book-keeping,
to record whether a define_subst still needs to be applied to a pattern.
So the attribute is set to "yes" before the subst is applied (on patterns
that need substituting) and is reset to "no" once the subst has been applied.
So as things stand, if you want to record when the substitution has been
applied, you'd need to define your own define_subst_attribute (with values
no/yes, false/true, or whatever) and then use "<...>" to substitute the
value into an explicit set_attr.
>
> Finally, to have both a template for zero_extend:DI and a template for
> sign_extend:DI to be generated from the original SI template, I have
> applied a double (define_subst ...) to the original "addsi3" template
> as follows:
>
> (define_subst_attr "arith_zx" "arith_zx_subst" "" "_zx")
> (define_subst "arith_zx_subst"
> [(set (match_operand:SI 0 "" "")
> (match_operand:SI 1 "" ""))]
> ""
> [(set (match_operand:DI 0 "register_operand" "=r")
> (zero_extend:DI (match_dup 1)))]
> )
>
> (define_subst_attr "arith_sx" "arith_sx_subst" "" "_sx")
> (define_subst_attr "modif_sx" "arith_sx_subst" "" ".sx")
> (define_subst "arith_sx_subst"
> [(set (match_operand:SI 0 "" "")
> (match_operand:SI 1 "" ""))]
> ""
> [(set (match_operand:DI 0 "register_operand" "=r")
> (sign_extend:DI (match_dup 1)))]
> )
>
> (define_insn "addsi3<arith_zx><arith_sx>"
> [(set (match_operand:SI 0 "register_operand" "=r,r,r")
> (plus:SI (match_operand:SI 1 "register_operand" "r,r,r")
> (match_operand:SI 2 "register_w32_operand" "r,I10,W32")))]
> ""
> "addw<modif_sx> %0 = %1, %2"
> [(set_attr "type" "alu_tiny,alu_tiny_w,alu_tiny_x")
> (set_attr "length" "4,4,8")]
> )
>
> While this produces the expected insn templates "addsi3", "addsi3_zx",
> "addsi3_sx", is this use of multiple (define_subst ...) is anticipated
> or correct?
Yeah, that's valid. aarch64 does the same thing.
> I have tried before combining a single (define_subst ...) with a code
> iterator ANY_EXTEND inside the output-template of (define_subst ...)
> to iterate zero_extend and sign_extend, but this did not work, as
> ANY_EXTEND was just replaced by the first in its list of operators.
Yeah, using iterators isn't currently supported. I suppose we should
error out on it.
Thanks,
Richard
