Re: [PATCH v1 2/6] LoongArch: Added Loongson SX base instruction support.

[WANG Xuerui <i@xen0n.name>]

On 2023/6/30 10:16, Chenghui Pan wrote:
> [snip]
> ---
>   gcc/config/loongarch/constraints.md        |  128 +-
>   gcc/config/loongarch/loongarch-builtins.cc |   10 +
>   gcc/config/loongarch/loongarch-modes.def   |   38 +
>   gcc/config/loongarch/loongarch-protos.h    |   31 +
>   gcc/config/loongarch/loongarch.cc          | 2235 +++++++++-
>   gcc/config/loongarch/loongarch.h           |   65 +-
>   gcc/config/loongarch/loongarch.md          |   44 +-
>   gcc/config/loongarch/lsx.md                | 4490 ++++++++++++++++++++
>   gcc/config/loongarch/predicates.md         |  333 +-
>   9 files changed, 7184 insertions(+), 190 deletions(-)
>   create mode 100644 gcc/config/loongarch/lsx.md
> 
> diff --git a/gcc/config/loongarch/constraints.md b/gcc/config/loongarch/constraints.md
> index 7a38cd07ae9..1dd56af07c4 100644
> --- a/gcc/config/loongarch/constraints.md
> +++ b/gcc/config/loongarch/constraints.md
> @@ -30,8 +30,7 @@
>   ;; "h" <-----unused
>   ;; "i" "Matches a general integer constant." (Global non-architectural)
>   ;; "j" SIBCALL_REGS
> -;; "k" "A memory operand whose address is formed by a base register and
> -;;      (optionally scaled) index register."
> +;; "k" <-----unused
>   ;; "l" "A signed 16-bit constant."
>   ;; "m" "A memory operand whose address is formed by a base register and offset
>   ;;      that is suitable for use in instructions with the same addressing mode
> @@ -80,13 +79,14 @@
>   ;; "N" <-----unused
>   ;; "O" <-----unused
>   ;; "P" <-----unused
> -;; "Q" <-----unused
> +;; "Q" "A signed 12-bit constant"
>   ;; "R" <-----unused
>   ;; "S" <-----unused
>   ;; "T" <-----unused
>   ;; "U" <-----unused
>   ;; "V" "Matches a non-offsettable memory reference." (Global non-architectural)
> -;; "W" <-----unused
> +;; "W" "A memory address based on a member of @code{BASE_REG_CLASS}.  This is
> +;;     true for all references."
>   ;; "X" "Matches anything." (Global non-architectural)
>   ;; "Y" -
>   ;;    "Yd"
> @@ -214,6 +214,63 @@ (define_constraint "Le"
>     (and (match_code "const_int")
>          (match_test "loongarch_addu16i_imm12_operand_p (ival, SImode)")))
>   
> +(define_constraint "M"
> +  "A constant that cannot be loaded using @code{lui}, @code{addiu}
> +   or @code{ori}."
> +  (and (match_code "const_int")
> +       (not (match_test "IMM12_OPERAND (ival)"))
> +       (not (match_test "IMM12_OPERAND_UNSIGNED (ival)"))
> +       (not (match_test "LU12I_OPERAND (ival)"))))
> +
> +(define_constraint "N"
> +  "A constant in the range -65535 to -1 (inclusive)."
> +  (and (match_code "const_int")
> +       (match_test "ival >= -0xffff && ival < 0")))
> +
> +(define_constraint "O"
> +  "A signed 15-bit constant."
> +  (and (match_code "const_int")
> +       (match_test "ival >= -0x4000 && ival < 0x4000")))
> +
> +(define_constraint "P"
> +  "A constant in the range 1 to 65535 (inclusive)."
> +  (and (match_code "const_int")
> +       (match_test "ival > 0 && ival < 0x10000")))

These constraints are meant to be exposed for developers to use, right? 
If not so they should probably be marked "@internal", and if so you 
should update the docs as well.

Also these are not documented in the comment block at the top of file.

> +
> +;; General constraints
> +
> +(define_memory_constraint "R"
> +  "An address that can be used in a non-macro load or store."
> +  (and (match_code "mem")
> +       (match_test "loongarch_address_insns (XEXP (op, 0), mode, false) == 1")))

Similarly, is this "R" constraint meant to be exposed as well? Sure 
one's free to choose letters but "R" IMO strongly implies something 
related to registers, not addresses...

> +(define_constraint "S"
> +  "@internal
> +   A constant call address."
> +  (and (match_operand 0 "call_insn_operand")
> +       (match_test "CONSTANT_P (op)")))

Additionally, IMO we probably should minimize our use of single-letter 
constraints that don't overlap with other architectures' similar usage. 
(I know that several projects have accepted LSX/LASX code well ahead of 
this series, but I don't know off my head if their code used any inline 
asm instead of C intrinsics. Intuitively this shouldn't be a concern 
though.)

Overall, I'd recommend moving all single-letter constraints added here 
to a two-letter namespace, so everything is better namespaced and easier 
to remember (e.g. if we choose something like "Vx" or "Yx" for 
everything vector-related, it'd be a lot easier to mentally associate 
the two-letter incantations with correct semantics.)

> +
> +(define_constraint "YG"
> +  "@internal
> +   A vector zero."
> +  (and (match_code "const_vector")
> +       (match_test "op == CONST0_RTX (mode)")))
> +
> +(define_constraint "YA"
> +  "@internal
> +   An unsigned 6-bit constant."
> +  (and (match_code "const_int")
> +       (match_test "UIMM6_OPERAND (ival)")))
> +
> +(define_constraint "YB"
> +  "@internal
> +   A signed 10-bit constant."
> +  (and (match_code "const_int")
> +       (match_test "IMM10_OPERAND (ival)")))
> +
> +(define_constraint "Yb"
> +   "@internal"
> +   (match_operand 0 "qi_mask_operand"))
> +
>   (define_constraint "Yd"
>     "@internal
>      A constant @code{move_operand} that can be safely loaded using
> @@ -221,10 +278,73 @@ (define_constraint "Yd"
>     (and (match_operand 0 "move_operand")
>          (match_test "CONSTANT_P (op)")))
>   
> +(define_constraint "Yh"
> +   "@internal"
> +    (match_operand 0 "hi_mask_operand"))
> +
> +(define_constraint "Yw"
> +   "@internal"
> +    (match_operand 0 "si_mask_operand"))
> +
>   (define_constraint "Yx"
>      "@internal"
>      (match_operand 0 "low_bitmask_operand"))
>   
> +(define_constraint "YI"
> +  "@internal
> +   A replicated vector const in which the replicated value is in the range
> +   [-512,511]."
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_same_int_p (op, mode, -512, 511)")))
> +
> +(define_constraint "YC"
> +  "@internal
> +   A replicated vector const in which the replicated value has a single
> +   bit set."
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_bitimm_set_p (op, mode)")))
> +
> +(define_constraint "YZ"
> +  "@internal
> +   A replicated vector const in which the replicated value has a single
> +   bit clear."
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_bitimm_clr_p (op, mode)")))
> +
> +(define_constraint "Unv5"
> +  "@internal
> +   A replicated vector const in which the replicated value is in the range
> +   [-31,0]."
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_same_int_p (op, mode, -31, 0)")))
> +
> +(define_constraint "Uuv5"
> +  "@internal
> +   A replicated vector const in which the replicated value is in the range
> +   [0,31]."
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_same_int_p (op, mode, 0, 31)")))
> +
> +(define_constraint "Usv5"
> +  "@internal
> +   A replicated vector const in which the replicated value is in the range
> +   [-16,15]."
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_same_int_p (op, mode, -16, 15)")))
> +
> +(define_constraint "Uuv6"
> +  "@internal
> +   A replicated vector const in which the replicated value is in the range
> +   [0,63]."
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_same_int_p (op, mode, 0, 63)")))
> +
> +(define_constraint "Urv8"
> +  "@internal
> +   A replicated vector const with replicated byte values as well as elements"
> +  (and (match_code "const_vector")
> +       (match_test "loongarch_const_vector_same_bytes_p (op, mode)")))
> +
>   (define_memory_constraint "ZC"
>     "A memory operand whose address is formed by a base register and offset
>      that is suitable for use in instructions with the same addressing mode
> [snip]

I didn't review any further, partly because I'm only doing this in my 
heavily fragmented free time, and partly because I'd agree with Ruoyao 
-- the patch is huge and will certainly benefit from some simple 
separation of logical changes.