Re: [PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low Overhead Loops
On 30/01/2024 14:09, Andre Simoes Dias Vieira wrote: > Hi Richard, > > Thanks for the reviews, I'm making these changes but just a heads up. > > When hardcoding LR_REGNUM like this we need to change the way we compare the > register in doloop_condition_get. This function currently compares the rtx > nodes by address, which I think happens to be fine before we assign hard > registers, as I suspect we always share the rtx node for the same pseudo, but > when assigning registers it seems like we create copies, so things like: > `XEXP (inc_src, 0) == reg` will fail for > inc_src: (plus (reg LR) (const_int -n)' > reg: (reg LR) > > Instead I will substitute the operand '==' with calls to 'rtx_equal_p (op1, > op2, NULL)'. Yes, that's fine. R. > > Sound good? > > Kind regards, > Andre > > > From: Richard Earnshaw (lists) > Sent: Tuesday, January 30, 2024 11:36 AM > To: Andre Simoes Dias Vieira; gcc-patches@gcc.gnu.org > Cc: Kyrylo Tkachov; Stam Markianos-Wright > Subject: Re: [PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low > Overhead Loops > > On 19/01/2024 14:40, Andre Vieira wrote: >> >> Respin after comments from Kyrill and rebase. I also removed an if-then-else >> construct in arm_mve_check_reg_origin_is_num_elems similar to the other >> functions >> Kyrill pointed out. >> >> After an earlier comment from Richard Sandiford I also added comments to the >> two tail predication patterns added to explain the need for the unspecs. > > [missing ChangeLog] > > I'm just going to focus on loop-doloop.c in this reply, I'll respond to the > other bits in a follow-up. > > 2) (set (reg) (plus (reg) (const_int -1)) > - (set (pc) (if_then_else (reg != 0) > -(label_ref (label)) > -(pc))). > +(set (pc) (if_then_else (reg != 0) > +(label_ref (label)) > +(pc))). > > Some targets (ARM) do the comparison before the branch, as in the > following form: > > - 3) (parallel [(set (cc) (compare ((plus (reg) (const_int -1), 0))) > - (set (reg) (plus (reg) (const_int -1)))]) > -(set (pc) (if_then_else (cc == NE) > ... > > > This comment is becoming confusing. Really the text leading up to 3)... > should be inside 3. Something like: > > 3) Some targets (ARM) do the comparison before the branch, as in the > following form: > > (parallel [(set (cc) (compare (plus (reg) (const_int -1)) 0)) > (set (reg) (plus (reg) (const_int -1)))]) > (set (pc) (if_then_else (cc == NE) > (label_ref (label)) > (pc)))]) > > > The same issue on the comment structure also applies to the new point 4... > > + The ARM target also supports a special case of a counter that > decrements > + by `n` and terminating in a GTU condition. In that case, the compare > and > + branch are all part of one insn, containing an UNSPEC: > + > + 4) (parallel [ > + (set (pc) > + (if_then_else (gtu (unspec:SI [(plus:SI (reg:SI 14 lr) > + (const_int -n))]) > + (const_int n-1])) > + (label_ref) > + (pc))) > + (set (reg:SI 14 lr) > +(plus:SI (reg:SI 14 lr) > + (const_int -n))) > + */ > > I think this needs a bit more clarification. Specifically that this > construct supports a predicated vectorized do loop. Also, the placement of > the unspec inside the comparison is ugnly and unnecessary. It should be > sufficient to have the unspec inside a USE expression, which the mid-end can > then ignore entirely. So > > (parallel > [(set (pc) (if_then_else (gtu (plus (reg) (const_int -n)) >(const_int n-1)) > (label_ref) (pc))) > (set (reg) (plus (reg) (const_int -n))) > (additional clobbers and uses)]) > > For Arm, we then add a (use (unspec [(const_int 0)] N)) that is specific to > this pattern to stop anything else from matching it. > > Note that we don't need to mention that the register is 'LR' or the modes, > those are specific to a particular backend, not the generic pattern we want > to match. > > + || !CONST_INT_P (XEXP (inc_src, 1)) >
Re: [PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low Overhead Loops
Hi Richard,
Thanks for the reviews, I'm making these changes but just a heads up.
When hardcoding LR_REGNUM like this we need to change the way we compare the
register in doloop_condition_get. This function currently compares the rtx
nodes by address, which I think happens to be fine before we assign hard
registers, as I suspect we always share the rtx node for the same pseudo, but
when assigning registers it seems like we create copies, so things like:
`XEXP (inc_src, 0) == reg` will fail for
inc_src: (plus (reg LR) (const_int -n)'
reg: (reg LR)
Instead I will substitute the operand '==' with calls to 'rtx_equal_p (op1,
op2, NULL)'.
Sound good?
Kind regards,
Andre
From: Richard Earnshaw (lists)
Sent: Tuesday, January 30, 2024 11:36 AM
To: Andre Simoes Dias Vieira; gcc-patches@gcc.gnu.org
Cc: Kyrylo Tkachov; Stam Markianos-Wright
Subject: Re: [PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low
Overhead Loops
On 19/01/2024 14:40, Andre Vieira wrote:
>
> Respin after comments from Kyrill and rebase. I also removed an if-then-else
> construct in arm_mve_check_reg_origin_is_num_elems similar to the other
> functions
> Kyrill pointed out.
>
> After an earlier comment from Richard Sandiford I also added comments to the
> two tail predication patterns added to explain the need for the unspecs.
[missing ChangeLog]
I'm just going to focus on loop-doloop.c in this reply, I'll respond to the
other bits in a follow-up.
2) (set (reg) (plus (reg) (const_int -1))
- (set (pc) (if_then_else (reg != 0)
-(label_ref (label))
-(pc))).
+(set (pc) (if_then_else (reg != 0)
+(label_ref (label))
+(pc))).
Some targets (ARM) do the comparison before the branch, as in the
following form:
- 3) (parallel [(set (cc) (compare ((plus (reg) (const_int -1), 0)))
- (set (reg) (plus (reg) (const_int -1)))])
-(set (pc) (if_then_else (cc == NE)
...
This comment is becoming confusing. Really the text leading up to 3)... should
be inside 3. Something like:
3) Some targets (ARM) do the comparison before the branch, as in the
following form:
(parallel [(set (cc) (compare (plus (reg) (const_int -1)) 0))
(set (reg) (plus (reg) (const_int -1)))])
(set (pc) (if_then_else (cc == NE)
(label_ref (label))
(pc)))])
The same issue on the comment structure also applies to the new point 4...
+ The ARM target also supports a special case of a counter that decrements
+ by `n` and terminating in a GTU condition. In that case, the compare and
+ branch are all part of one insn, containing an UNSPEC:
+
+ 4) (parallel [
+ (set (pc)
+ (if_then_else (gtu (unspec:SI [(plus:SI (reg:SI 14 lr)
+ (const_int -n))])
+ (const_int n-1]))
+ (label_ref)
+ (pc)))
+ (set (reg:SI 14 lr)
+(plus:SI (reg:SI 14 lr)
+ (const_int -n)))
+ */
I think this needs a bit more clarification. Specifically that this construct
supports a predicated vectorized do loop. Also, the placement of the unspec
inside the comparison is ugnly and unnecessary. It should be sufficient to
have the unspec inside a USE expression, which the mid-end can then ignore
entirely. So
(parallel
[(set (pc) (if_then_else (gtu (plus (reg) (const_int -n))
(const_int n-1))
(label_ref) (pc)))
(set (reg) (plus (reg) (const_int -n)))
(additional clobbers and uses)])
For Arm, we then add a (use (unspec [(const_int 0)] N)) that is specific to
this pattern to stop anything else from matching it.
Note that we don't need to mention that the register is 'LR' or the modes,
those are specific to a particular backend, not the generic pattern we want to
match.
+ || !CONST_INT_P (XEXP (inc_src, 1))
+ || INTVAL (XEXP (inc_src, 1)) >= 0)
return 0;
+ int dec_num = abs (INTVAL (XEXP (inc_src, 1)));
We can just use '-INTVAL(...)' here, we've verified just above that the
constant is negative.
- if ((XEXP (condition, 0) == reg)
+ /* For the ARM special case of having a GTU: re-form the condition without
+ the unspec for the benefit of the middle-end. */
+ if (GET_CODE (condition) == GTU)
+{
+ condition = gen_rtx_fmt_ee (GTU, VOIDmode, inc_src,
+ GEN_INT (dec_num - 1));
+ return condition;
+}
If you make the change I mentioned above, this re-forming isn't needed a
Re: [PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low Overhead Loops
On 19/01/2024 14:40, Andre Vieira wrote:
>
> Respin after comments from Kyrill and rebase. I also removed an if-then-else
> construct in arm_mve_check_reg_origin_is_num_elems similar to the other
> functions
> Kyrill pointed out.
>
> After an earlier comment from Richard Sandiford I also added comments to the
> two tail predication patterns added to explain the need for the unspecs.
[missing ChangeLog]
I'm just going to focus on loop-doloop.c in this reply, I'll respond to the
other bits in a follow-up.
2) (set (reg) (plus (reg) (const_int -1))
- (set (pc) (if_then_else (reg != 0)
-(label_ref (label))
-(pc))).
+(set (pc) (if_then_else (reg != 0)
+(label_ref (label))
+(pc))).
Some targets (ARM) do the comparison before the branch, as in the
following form:
- 3) (parallel [(set (cc) (compare ((plus (reg) (const_int -1), 0)))
- (set (reg) (plus (reg) (const_int -1)))])
-(set (pc) (if_then_else (cc == NE)
...
This comment is becoming confusing. Really the text leading up to 3)... should
be inside 3. Something like:
3) Some targets (ARM) do the comparison before the branch, as in the
following form:
(parallel [(set (cc) (compare (plus (reg) (const_int -1)) 0))
(set (reg) (plus (reg) (const_int -1)))])
(set (pc) (if_then_else (cc == NE)
(label_ref (label))
(pc)))])
The same issue on the comment structure also applies to the new point 4...
+ The ARM target also supports a special case of a counter that decrements
+ by `n` and terminating in a GTU condition. In that case, the compare and
+ branch are all part of one insn, containing an UNSPEC:
+
+ 4) (parallel [
+ (set (pc)
+ (if_then_else (gtu (unspec:SI [(plus:SI (reg:SI 14 lr)
+ (const_int -n))])
+ (const_int n-1]))
+ (label_ref)
+ (pc)))
+ (set (reg:SI 14 lr)
+(plus:SI (reg:SI 14 lr)
+ (const_int -n)))
+ */
I think this needs a bit more clarification. Specifically that this construct
supports a predicated vectorized do loop. Also, the placement of the unspec
inside the comparison is ugnly and unnecessary. It should be sufficient to
have the unspec inside a USE expression, which the mid-end can then ignore
entirely. So
(parallel
[(set (pc) (if_then_else (gtu (plus (reg) (const_int -n))
(const_int n-1))
(label_ref) (pc)))
(set (reg) (plus (reg) (const_int -n)))
(additional clobbers and uses)])
For Arm, we then add a (use (unspec [(const_int 0)] N)) that is specific to
this pattern to stop anything else from matching it.
Note that we don't need to mention that the register is 'LR' or the modes,
those are specific to a particular backend, not the generic pattern we want to
match.
+ || !CONST_INT_P (XEXP (inc_src, 1))
+ || INTVAL (XEXP (inc_src, 1)) >= 0)
return 0;
+ int dec_num = abs (INTVAL (XEXP (inc_src, 1)));
We can just use '-INTVAL(...)' here, we've verified just above that the
constant is negative.
- if ((XEXP (condition, 0) == reg)
+ /* For the ARM special case of having a GTU: re-form the condition without
+ the unspec for the benefit of the middle-end. */
+ if (GET_CODE (condition) == GTU)
+{
+ condition = gen_rtx_fmt_ee (GTU, VOIDmode, inc_src,
+ GEN_INT (dec_num - 1));
+ return condition;
+}
If you make the change I mentioned above, this re-forming isn't needed any
more, so the arm-specific comment goes away
- {
+{
if (GET_CODE (pattern) != PARALLEL)
/* For the second form we expect:
You've fixed the indentation of the brace (good), but the body of the braced
expression needs re-indenting as well.
R.
[PATCH v3 2/2] arm: Add support for MVE Tail-Predicated Low Overhead Loops
Respin after comments from Kyrill and rebase. I also removed an if-then-else
construct in arm_mve_check_reg_origin_is_num_elems similar to the other
functions
Kyrill pointed out.
After an earlier comment from Richard Sandiford I also added comments to the
two tail predication patterns added to explain the need for the unspecs.
diff --git a/gcc/config/arm/arm-protos.h b/gcc/config/arm/arm-protos.h
index 2cd560c9925..76c6ee95c16 100644
--- a/gcc/config/arm/arm-protos.h
+++ b/gcc/config/arm/arm-protos.h
@@ -65,8 +65,8 @@ extern void arm_emit_speculation_barrier_function (void);
extern void arm_decompose_di_binop (rtx, rtx, rtx *, rtx *, rtx *, rtx *);
extern bool arm_q_bit_access (void);
extern bool arm_ge_bits_access (void);
-extern bool arm_target_insn_ok_for_lob (rtx);
-
+extern bool arm_target_bb_ok_for_lob (basic_block);
+extern rtx arm_attempt_dlstp_transform (rtx);
#ifdef RTX_CODE
enum reg_class
arm_mode_base_reg_class (machine_mode);
diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc
index e5a944486d7..75432f3f73a 100644
--- a/gcc/config/arm/arm.cc
+++ b/gcc/config/arm/arm.cc
@@ -668,6 +668,12 @@ static const scoped_attribute_specs *const arm_attribute_table[] =
#undef TARGET_HAVE_CONDITIONAL_EXECUTION
#define TARGET_HAVE_CONDITIONAL_EXECUTION arm_have_conditional_execution
+#undef TARGET_LOOP_UNROLL_ADJUST
+#define TARGET_LOOP_UNROLL_ADJUST arm_loop_unroll_adjust
+
+#undef TARGET_PREDICT_DOLOOP_P
+#define TARGET_PREDICT_DOLOOP_P arm_predict_doloop_p
+
#undef TARGET_LEGITIMATE_CONSTANT_P
#define TARGET_LEGITIMATE_CONSTANT_P arm_legitimate_constant_p
@@ -34483,19 +34489,1135 @@ arm_invalid_within_doloop (const rtx_insn *insn)
}
bool
-arm_target_insn_ok_for_lob (rtx insn)
+arm_target_bb_ok_for_lob (basic_block bb)
{
- basic_block bb = BLOCK_FOR_INSN (insn);
/* Make sure the basic block of the target insn is a simple latch
having as single predecessor and successor the body of the loop
itself. Only simple loops with a single basic block as body are
supported for 'low over head loop' making sure that LE target is
above LE itself in the generated code. */
-
return single_succ_p (bb)
-&& single_pred_p (bb)
-&& single_succ_edge (bb)->dest == single_pred_edge (bb)->src
-&& contains_no_active_insn_p (bb);
+ && single_pred_p (bb)
+ && single_succ_edge (bb)->dest == single_pred_edge (bb)->src;
+}
+
+/* Utility fuction: Given a VCTP or a VCTP_M insn, return the number of MVE
+ lanes based on the machine mode being used. */
+
+static int
+arm_mve_get_vctp_lanes (rtx_insn *insn)
+{
+ rtx insn_set = single_set (insn);
+ if (insn_set
+ && GET_CODE (SET_SRC (insn_set)) == UNSPEC
+ && (XINT (SET_SRC (insn_set), 1) == VCTP
+ || XINT (SET_SRC (insn_set), 1) == VCTP_M))
+{
+ machine_mode mode = GET_MODE (SET_SRC (insn_set));
+ return (VECTOR_MODE_P (mode) && VALID_MVE_PRED_MODE (mode))
+ ? GET_MODE_NUNITS (mode) : 0;
+}
+ return 0;
+}
+
+/* Check if INSN requires the use of the VPR reg, if it does, return the
+ sub-rtx of the VPR reg. The TYPE argument controls whether
+ this function should:
+ * For TYPE == 0, check all operands, including the OUT operands,
+ and return the first occurrence of the VPR reg.
+ * For TYPE == 1, only check the input operands.
+ * For TYPE == 2, only check the output operands.
+ (INOUT operands are considered both as input and output operands)
+*/
+static rtx
+arm_get_required_vpr_reg (rtx_insn *insn, unsigned int type = 0)
+{
+ gcc_assert (type < 3);
+ if (!NONJUMP_INSN_P (insn))
+return NULL_RTX;
+
+ bool requires_vpr;
+ extract_constrain_insn (insn);
+ int n_operands = recog_data.n_operands;
+ if (recog_data.n_alternatives == 0)
+return NULL_RTX;
+
+ /* Fill in recog_op_alt with information about the constraints of
+ this insn. */
+ preprocess_constraints (insn);
+
+ for (int op = 0; op < n_operands; op++)
+{
+ requires_vpr = true;
+ if (type == 1 && recog_data.operand_type[op] == OP_OUT)
+ continue;
+ else if (type == 2 && recog_data.operand_type[op] == OP_IN)
+ continue;
+
+ /* Iterate through alternatives of operand "op" in recog_op_alt and
+ identify if the operand is required to be the VPR. */
+ for (int alt = 0; alt < recog_data.n_alternatives; alt++)
+ {
+ const operand_alternative *op_alt
+ = &recog_op_alt[alt * n_operands];
+ /* Fetch the reg_class for each entry and check it against the
+ VPR_REG reg_class. */
+ if (alternative_class (op_alt, op) != VPR_REG)
+ requires_vpr = false;
+ }
+ /* If all alternatives of the insn require the VPR reg for this operand,
+ it means that either this is VPR-generating instruction, like a vctp,
+ vcmp, etc., or it is a VPT-predicated insruction. Return the subrtx
+ of the VPR reg operand. */
+ if (requires_vpr)
+ return recog_data.operand[op];
+}
+ return NULL_RTX;
+}
+
+/* Wrapper function of arm_get_required_vpr_reg
