Karl Meakin <karl.mea...@arm.com> writes:
> Add rules for lowering `cbranch<mode>4` to CBB<cond>/CBH<cond>/CB<cond> when
> CMPBR extension is enabled.
>
> gcc/ChangeLog:
>
> * config/aarch64/aarch64-protos.h (aarch64_cb_rhs): New function.
> * config/aarch64/aarch64.cc (aarch64_cb_rhs): Likewise.
> * config/aarch64/aarch64.md (cbranch<mode>4): Rename to ...
> (cbranch<GPI:mode>4): ...here, and emit CMPBR if possible.
> (cbranch<SHORT:mode>4): New expand rule.
> (aarch64_cb<INT_CMP:code><GPI:mode>): New insn rule.
> (aarch64_cb<INT_CMP:code><SHORT:mode>): Likewise.
> * config/aarch64/constraints.md (Uc0): New constraint.
> (Uc1): Likewise.
> (Uc2): Likewise.
> * config/aarch64/iterators.md (cmpbr_suffix): New mode attr.
> (INT_CMP): New code iterator.
> (cmpbr_imm_constraint): New code attr.
> * config/aarch64/predicates.md (const_0_to_63_operand): New predicate.
> (aarch64_cb_immediate): Likewise.
> (aarch64_cb_operand): Likewise.
> (aarch64_cb_short_operand): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/aarch64/cmpbr.c:
> ---
> gcc/config/aarch64/aarch64-protos.h | 2 +
> gcc/config/aarch64/aarch64.cc | 33 ++
> gcc/config/aarch64/aarch64.md | 89 +++-
> gcc/config/aarch64/constraints.md | 18 +
> gcc/config/aarch64/iterators.md | 19 +
> gcc/config/aarch64/predicates.md | 15 +
> gcc/testsuite/gcc.target/aarch64/cmpbr.c | 586 ++++++++---------------
> 7 files changed, 376 insertions(+), 386 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index 31f2f5b8bd2..0f104d0641b 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -1135,6 +1135,8 @@ bool aarch64_general_check_builtin_call (location_t,
> vec<location_t>,
> unsigned int, tree, unsigned int,
> tree *);
>
> +bool aarch64_cb_rhs (rtx op, rtx rhs);
> +
> namespace aarch64 {
> void report_non_ice (location_t, tree, unsigned int);
> void report_out_of_range (location_t, tree, unsigned int, HOST_WIDE_INT,
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 667e42ba401..3dc139e9a72 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -959,6 +959,39 @@ svpattern_token (enum aarch64_svpattern pattern)
> gcc_unreachable ();
> }
>
> +/* Return true if rhs is an operand suitable for a CB<cc> (immediate)
> + instruction. */
This should also mention what "op" is. The convention is also to use caps
to refer to parameter names.
Maybe:
/* Return true if RHS is an immediate operand suitable for a CB<cc> (immediate)
instruction. OP determines the type of the comparison. */
> +bool
> +aarch64_cb_rhs (rtx op, rtx rhs)
> +{
> + if (!CONST_INT_P (rhs))
> + return REG_P (rhs);
> +
> + HOST_WIDE_INT rhs_val = INTVAL (rhs);
> +
> + switch (GET_CODE (op))
> + {
> + case EQ:
> + case NE:
> + case GT:
> + case GTU:
> + case LT:
> + case LTU:
> + return IN_RANGE (rhs_val, 0, 63);
> +
> + case GE: /* CBGE: signed greater than or equal */
> + case GEU: /* CBHS: unsigned greater than or equal */
> + return IN_RANGE (rhs_val, 1, 64);
> +
> + case LE: /* CBLE: signed less than or equal */
> + case LEU: /* CBLS: unsigned less than or equal */
> + return IN_RANGE (rhs_val, -1, 62);
> +
> + default:
> + return false;
> + }
> +}
> +
> /* Return the location of a piece that is known to be passed or returned
> in registers. FIRST_ZR is the first unused vector argument register
> and FIRST_PR is the first unused predicate argument register. */
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 0a378ab377d..23bce55f620 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -717,6 +717,10 @@ (define_constants
> ;; +/- 32KiB. Used by TBZ, TBNZ.
> (BRANCH_LEN_P_32KiB 32764)
> (BRANCH_LEN_N_32KiB -32768)
> +
> + ;; +/- 1KiB. Used by CBB<cond>, CBH<cond>, CB<cond>.
> + (BRANCH_LEN_P_1Kib 1020)
> + (BRANCH_LEN_N_1Kib -1024)
> ]
> )
>
> @@ -724,18 +728,35 @@ (define_constants
> ;; Conditional jumps
> ;; -------------------------------------------------------------------
>
> -(define_expand "cbranch<mode>4"
> +(define_expand "cbranch<GPI:mode>4"
> [(set (pc) (if_then_else (match_operator 0 "aarch64_comparison_operator"
> [(match_operand:GPI 1 "register_operand")
> (match_operand:GPI 2 "aarch64_plus_operand")])
> (label_ref (match_operand 3))
> (pc)))]
> ""
> - "
> - operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]), operands[1],
> - operands[2]);
> - operands[2] = const0_rtx;
> - "
> + {
> + if (TARGET_CMPBR && aarch64_cb_rhs(operands[0], operands[2]))
> + {
> + // Fall-through to `aarch64_cbranch`
> + }
> + else
> + {
> + operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]),
> + operands[1], operands[2]);
> + operands[2] = const0_rtx;
> + }
> + }
Sorry for the formatting nits, but: the body should be indented 2 spaces
relative to the "{". I think the convention is to use /* ... */ comments
in files that already use them.
Also, "aarch64_cbranch" might be confusing, since the name is
aarch64_cb<INT_CMP:code><GPI:mode>.
So maybe:
{
if (TARGET_CMPBR && aarch64_cb_rhs(operands[0], operands[2]))
{
/* Fall through to `aarch64_cb<INT_CMP:code><GPI:mode>`. */
}
else
{
operands[1] = aarch64_gen_compare_reg (GET_CODE (operands[0]),
operands[1], operands[2]);
operands[2] = const0_rtx;
}
}
> +)
> +
> +(define_expand "cbranch<SHORT:mode>4"
> + [(set (pc) (if_then_else (match_operator 0 "aarch64_comparison_operator"
> + [(match_operand:SHORT 1 "register_operand")
> + (match_operand:SHORT 2
> "aarch64_cb_short_operand")])
There's no need to define a new predicate for this. We can just
reuse aarch64_reg_or_zero.
> + (label_ref (match_operand 3))
> + (pc)))]
> + "TARGET_CMPBR"
> + ""
> )
>
> (define_expand "cbranch<mode>4"
> @@ -762,6 +783,62 @@ (define_expand "cbranchcc4"
> ""
> )
>
> +;; Emit a `CB<cond> (register)` or `CB<cond> (immediate)` instruction.
> +;; Only immediates in the range 0-63 are supported.
Maybe better as "The immediate range depends on the comparison code."
> +;; Comparisons against immediates outside this range fall back to
> +;; CMP + B<cond>.
> +(define_insn "aarch64_cb<INT_CMP:code><GPI:mode>"
> + [(set (pc) (if_then_else (INT_CMP
> + (match_operand:GPI 0 "register_operand" "r")
> + (match_operand:GPI 1 "aarch64_cb_operand"
The predicate needs to match everything that the constraints do, whereas
this only accepts [0, 63] for all codes.
There are two ways of coping with that:
(1) Have three predicates: one for [0, 63], one for [1, 64], and one
for [-1, 62].
The simplest way of doing that would probably be to rename
"cmpbr_imm_constraint" to something like "cmpbr_int_range" and
make it expand to the last character of the constraint, with
"rUc<INT_CMP:cmpbr_imm_range>" used below. Then the predicate could
also use "aarch64_cb_operand<<INT_CMP:cmpbr_imm_range>".
(2) (a) Change aarch64_cb_rhs so that it takes an rtx_code rather than
an rtx for the operator.
(b) Add:
&& aarch64_cb_rhs (<INT_CMP:CODE>, operands[1])
to the insn condition (after TARGET_CMPBR)
(c) Change the predicate to nonmemory_operand, since all range checking
is now done by the insn condition.
(2) seems simpler, since the patch would then not need any new predicates.
So maybe go for that.
Sorry that this is so clunky :(
Otherwise the patch looks really good.
Thanks,
Richard
> + "r<INT_CMP:cmpbr_imm_constraint>"))
> + (label_ref (match_operand 2))
> + (pc)))]
> + "TARGET_CMPBR"
> + "cb<INT_CMP:cmp_op>\\t%<w>0, %<w>1, %l2";
> + [(set_attr "type" "branch")
> + (set (attr "length")
> + (if_then_else (and (ge (minus (match_dup 2) (pc))
> + (const_int BRANCH_LEN_N_1Kib))
> + (lt (minus (match_dup 2) (pc))
> + (const_int BRANCH_LEN_P_1Kib)))
> + (const_int 4)
> + (const_int 8)))
> + (set (attr "far_branch")
> + (if_then_else (and (ge (minus (match_dup 2) (pc))
> + (const_int BRANCH_LEN_N_1Kib))
> + (lt (minus (match_dup 2) (pc))
> + (const_int BRANCH_LEN_P_1Kib)))
> + (const_string "no")
> + (const_string "yes")))]
> +)
