Hi Andre,
> -----Original Message-----
> From: Andre Vieira <andre.simoesdiasvie...@arm.com>
> Sent: Friday, January 5, 2024 5:52 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Richard Earnshaw <richard.earns...@arm.com>; Stam Markianos-Wright
> <stam.markianos-wri...@arm.com>
> Subject: [PATCH v2 2/2] arm: Add support for MVE Tail-Predicated Low Overhead
> Loops
>
> Respin after comments on first version.
I think I'm nitpicking some code style and implementation points rather than
diving deep into the algorithms, I think those were okay last time I looked at
this some time ago.
+/* Return true if INSN is a MVE instruction that is VPT-predicable, but in
+ its unpredicated form, or if it is predicated, but on a predicate other
+ than VPR_REG. */
+
+static bool
+arm_mve_vec_insn_is_unpredicated_or_uses_other_predicate (rtx_insn *insn,
+ rtx vpr_reg)
+{
+ rtx insn_vpr_reg_operand;
+ if (MVE_VPT_UNPREDICATED_INSN_P (insn)
+ || (MVE_VPT_PREDICATED_INSN_P (insn)
+ && (insn_vpr_reg_operand = arm_get_required_vpr_reg_param (insn))
+ && !rtx_equal_p (vpr_reg, insn_vpr_reg_operand)))
+ return true;
+ else
+ return false;
+}
+
+/* Return true if INSN is a MVE instruction that is VPT-predicable and is
+ predicated on VPR_REG. */
+
+static bool
+arm_mve_vec_insn_is_predicated_with_this_predicate (rtx_insn *insn,
+ rtx vpr_reg)
+{
+ rtx insn_vpr_reg_operand;
+ if (MVE_VPT_PREDICATED_INSN_P (insn)
+ && (insn_vpr_reg_operand = arm_get_required_vpr_reg_param (insn))
+ && rtx_equal_p (vpr_reg, insn_vpr_reg_operand))
+ return true;
+ else
+ return false;
+}
These two functions seem to have an "if (condition) return true; else return
false;" structure that we try to avoid. How about:
rtx_insn vpr_reg_operand = MVE_VPT_PREDICATED_INSN_P (insn) ?
arm_get_required_vpr_reg_param (insn) : NULL_RTX;
return vpr_reg_operand && rtx_equal_p (vpr_reg, insn_vpr_reg_operand);
+static bool
+arm_is_mve_across_vector_insn (rtx_insn* insn)
+{
+ df_ref insn_defs = NULL;
+ if (!MVE_VPT_PREDICABLE_INSN_P (insn))
+ return false;
+
+ bool is_across_vector = false;
+ FOR_EACH_INSN_DEF (insn_defs, insn)
+ if (!VALID_MVE_MODE (GET_MODE (DF_REF_REG (insn_defs)))
+ && !arm_get_required_vpr_reg_ret_val (insn))
+ is_across_vector = true;
+
You can just return true here immediately, no need to set is_across_vector....
+ return is_across_vector;
... and you can return false here, avoiding the need for is_across_vector
entirely
+}
+static bool
+arm_mve_check_reg_origin_is_num_elems (basic_block body, rtx reg, rtx
vctp_step)
+{
+ /* Ok, we now know the loop starts from zero and increments by one.
+ Now just show that the max value of the counter came from an
+ appropriate ASHIFRT expr of the correct amount. */
+ basic_block pre_loop_bb = body->prev_bb;
+ while (pre_loop_bb && BB_END (pre_loop_bb)
+ && !df_bb_regno_only_def_find (pre_loop_bb, REGNO (reg)))
+ pre_loop_bb = pre_loop_bb->prev_bb;
+
+ df_ref counter_max_last_def = df_bb_regno_only_def_find (pre_loop_bb, REGNO
(reg));
+ if (!counter_max_last_def)
+ return false;
+ rtx counter_max_last_set = single_set (DF_REF_INSN (counter_max_last_def));
+ if (!counter_max_last_set)
+ return false;
+
+ /* If we encounter a simple SET from a REG, follow it through. */
+ if (REG_P (SET_SRC (counter_max_last_set)))
+ return arm_mve_check_reg_origin_is_num_elems
+ (pre_loop_bb->next_bb, SET_SRC (counter_max_last_set), vctp_step);
+
+ /* If we encounter a SET from an IF_THEN_ELSE where one of the operands is a
+ constant and the other is a REG, follow through to that REG. */
+ if (GET_CODE (SET_SRC (counter_max_last_set)) == IF_THEN_ELSE
+ && REG_P (XEXP (SET_SRC (counter_max_last_set), 1))
+ && CONST_INT_P (XEXP (SET_SRC (counter_max_last_set), 2)))
+ return arm_mve_check_reg_origin_is_num_elems
+ (pre_loop_bb->next_bb, XEXP (SET_SRC (counter_max_last_set), 1),
vctp_step);
+
+ if (GET_CODE (SET_SRC (counter_max_last_set)) == ASHIFTRT
+ && CONST_INT_P (XEXP (SET_SRC (counter_max_last_set), 1))
+ && ((1 << INTVAL (XEXP (SET_SRC (counter_max_last_set), 1)))
+ == abs (INTVAL (vctp_step))))
I'm a bit concerned here with using abs() for HOST_WIDE_INT values that are
compared to other HOST_WIDE_INT values.
abs () will implicitly cast the argument and return an int. We should use the
abs_hwi function defined in hwint.h. It may not cause problems in practice
given the ranges involved, but better safe than sorry at this stage.
Looks decent to me otherwise, and an impressive piece of work, thanks.
I'd give Richard an opportunity to comment next week when he's back before
committing though.
Thanks,
Kyrill
