Re: [PATCH 2/2] arm: Add support for MVE Tail-Predicated Low Overhead Loops

[Stamatis Markianos-Wright <stam.markianos-wright@arm.com>]

Thank you Andre for reviewing! I'll attach the updated version of the 
patch to the third review email (your final one thus far ;)

On 22/06/2023 16:54, Andre Vieira (lists) wrote:
> Some comments below, all quite minor. I'll continue to review 
> tomorrow, I need a fresher brain for 
> arm_mve_check_df_chain_back_for_implic_predic  ;)
>
> +static int
> +arm_mve_get_vctp_lanes (rtx x)
> +{
> +  if (GET_CODE (x) == SET && GET_CODE (XEXP (x, 1)) == UNSPEC
> +      && (XINT (XEXP (x, 1), 1) == VCTP || XINT (XEXP (x, 1), 1) == 
> VCTP_M))
> +    {
> +      switch (GET_MODE (XEXP (x, 1)))
> +    {
> +      case V16BImode:
> +        return 16;
> +      case V8BImode:
> +        return 8;
> +      case V4BImode:
> +        return 4;
> +      case V2QImode:
> +        return 2;
> +      default:
> +        break;
> +    }
> +    }
> +  return 0;
> +}
>
> I think you can replace the switch with something along the lines of:
> machine_mode mode = GET_MODE (XEXP (x, 1));
> return VECTOR_MODE_P (mode) ? GET_MODE_NUNITS (mode) : 0;
>
Ah true, especially now that there are no HImode predicates!

I added an additional check of `&& VALID_MVE_PRED_MODE (mode)` as well, 
just to make sure we could never pick up V4SImode, etc. (although I'd 
never expect that to happen if `rtx x` came from a valid instruction)


>
> +/* Check if an 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 occurance of the VPR_REG.
>
> s/occurance/occurrence/
Done
>
> +      bool requires_vpr;
> +  extract_constrain_insn (insn);
>
> indent of requires_vpr is off.
Done
>
> +      if (type == 1 && (recog_data.operand_type[op] == OP_OUT
> +            || recog_data.operand_type[op] == OP_INOUT))
> +    continue;
> +      else if (type == 2 && (recog_data.operand_type[op] == OP_IN
> +                 || recog_data.operand_type[op] == OP_INOUT))
> +    continue;
>
> Why skip INOUT? I guess this will become clear when I see the uses, 
> but I'm wondering whether 'only check the input operands.' is clear 
> enough. Maybe 'check operands that are input only.' would be more 
> accurate?
Oh! Thanks for spotting this. It also doesn't work with my comment at 
the top:
`(INOUT operands are considered both as input and output operands)`

It's been a long time since I wrote this piece, but it might be that I 
added this after realising that there are no insns with an OP_INOUT VPR 
reg. Since I don't think it's functional, I changed the code to align 
with the comment, instead.

>
> +      /* Fetch the reg_class for each entry and check it against the
> +       * VPR_REG reg_class.  */
>
> Remove leading * on the second line.
Damn auto-formatters ;)
Done
>
> +
> +/* Wrapper function of arm_get_required_vpr_reg with type == 1, so 
> return
> +   something only if the VPR reg is an input operand to the insn.  */
>
> When talking about a function parameter in comments capitalize (INSN) 
> the name. Same for:
Done
>
> +/* Wrapper function of arm_get_required_vpr_reg with type == 2, so 
> return
> +   something only if the VPR reg is the retrurn value, an output of, 
> or is
> +   clobbered by the insn.  */
>
> +/* Return true if an insn is an MVE instruction that VPT-predicable, 
> but in
> +   its unpredicated form, or if it is predicated, but on a predicate 
> other
> +   than vpr_reg.  */
>
> In this one also 'is a MVE instruction that is VPT-predicable' would 
> be better I think.
Oops, thanks for spotting. Done.
>
>
> On 15/06/2023 12:47, Stamatis Markianos-Wright via Gcc-patches wrote:
> >      Hi all,
> >
> >      This is the 2/2 patch that contains the functional changes needed
> >      for MVE Tail Predicated Low Overhead Loops.  See my previous email
> >      for a general introduction of MVE LOLs.
> >
> >      This support is added through the already existing loop-doloop
> >      mechanisms that are used for non-MVE dls/le looping.
> >
> >      Mid-end changes are:
> >
> >      1) Relax the loop-doloop mechanism in the mid-end to allow for
> >         decrement numbers other that -1 and for `count` to be an
> >         rtx containing a simple REG (which in this case will contain
> >         the number of elements to be processed), rather
> >         than an expression for calculating the number of iterations.
> >      2) Added a new df utility function: `df_bb_regno_only_def_find` 
> that
> >         will return the DEF of a REG only if it is DEF-ed once 
> within the
> >         basic block.
> >
> >      And many things in the backend to implement the above 
> optimisation:
> >
> >      3)  Implement the `arm_predict_doloop_p` target hook to 
> instruct the
> >          mid-end about Low Overhead Loops (MVE or not), as well as
> >          `arm_loop_unroll_adjust` which will prevent unrolling of 
> any loops
> >          that are valid for becoming MVE Tail_Predicated Low 
> Overhead Loops
> >          (unrolling can transform a loop in ways that invalidate the 
> dlstp/
> >          letp tranformation logic and the benefit of the dlstp/letp 
> loop
> >          would be considerably higher than that of unrolling)
> >      4)  Appropriate changes to the define_expand of doloop_end, new
> >          patterns for dlstp and letp, new iterators, unspecs, etc.
> >      5) `arm_mve_loop_valid_for_dlstp` and a number of checking 
> functions:
> >         * `arm_mve_dlstp_check_dec_counter`
> >         * `arm_mve_dlstp_check_inc_counter`
> >         * `arm_mve_check_reg_origin_is_num_elems`
> >         * `arm_mve_check_df_chain_back_for_implic_predic`
> >         * `arm_mve_check_df_chain_fwd_for_implic_predic_impact`
> >         This all, in smoe way or another, are running checks on the 
> loop
> >         structure in order to determine if the loop is valid for 
> dlstp/letp
> >         transformation.
> >      6) `arm_attempt_dlstp_transform`: (called from the 
> define_expand of
> >          doloop_end) this function re-checks for the loop's 
> suitability for
> >          dlstp/letp transformation and then implements it, if possible.
> >      7) Various utility functions:
> >         *`arm_mve_get_vctp_lanes` to map
> >         from vctp unspecs to number of lanes, and
> > `arm_get_required_vpr_reg`
> >         to check an insn to see if it requires the VPR or not.
> >         * `arm_mve_get_loop_vctp`
> >         * `arm_mve_get_vctp_lanes`
> >         * `arm_emit_mve_unpredicated_insn_to_seq`
> >         * `arm_get_required_vpr_reg`
> >         * `arm_get_required_vpr_reg_param`
> >         * `arm_get_required_vpr_reg_ret_val`
> >         * `arm_mve_vec_insn_is_predicated_with_this_predicate`
> >         * `arm_mve_vec_insn_is_unpredicated_or_uses_other_predicate`
> >
> >      No regressions on arm-none-eabi with various targets and on
> >      aarch64-none-elf. Thoughts on getting this into trunk?
> >
> >      Thank you,
> >      Stam Markianos-Wright
> >
> >      gcc/ChangeLog:
> >
> >              * config/arm/arm-protos.h (arm_target_insn_ok_for_lob):
> > Rename to...
> >              (arm_target_bb_ok_for_lob): ...this
> >              (arm_attempt_dlstp_transform): New.
> >              * config/arm/arm.cc (TARGET_LOOP_UNROLL_ADJUST): New.
> >              (TARGET_PREDICT_DOLOOP_P): New.
> >              (arm_block_set_vect):
> >              (arm_target_insn_ok_for_lob): Rename from
> > arm_target_insn_ok_for_lob.
> >              (arm_target_bb_ok_for_lob): New.
> >              (arm_mve_get_vctp_lanes): New.
> >              (arm_get_required_vpr_reg): New.
> >              (arm_get_required_vpr_reg_param): New.
> >              (arm_get_required_vpr_reg_ret_val): New.
> >              (arm_mve_get_loop_vctp): New.
> > (arm_mve_vec_insn_is_unpredicated_or_uses_other_predicate): New.
> > (arm_mve_vec_insn_is_predicated_with_this_predicate): New.
> >              (arm_mve_check_df_chain_back_for_implic_predic): New.
> > (arm_mve_check_df_chain_fwd_for_implic_predic_impact): New.
> >              (arm_mve_check_reg_origin_is_num_elems): New.
> >              (arm_mve_dlstp_check_inc_counter): New.
> >              (arm_mve_dlstp_check_dec_counter): New.
> >              (arm_mve_loop_valid_for_dlstp): New.
> >              (arm_predict_doloop_p): New.
> >              (arm_loop_unroll_adjust): New.
> >              (arm_emit_mve_unpredicated_insn_to_seq): New.
> >              (arm_attempt_dlstp_transform): New.
> >              * config/arm/iterators.md (DLSTP): New.
> >              (mode1): Add DLSTP mappings.
> >              * config/arm/mve.md (*predicated_doloop_end_internal): 
> New.
> >              (dlstp<mode1>_insn): New.
> >              * config/arm/thumb2.md (doloop_end): Update for MVE LOLs.
> >              * config/arm/unspecs.md: New unspecs.
> >              * df-core.cc (df_bb_regno_only_def_find): New.
> >              * df.h (df_bb_regno_only_def_find): New.
> >              * loop-doloop.cc (doloop_condition_get): Relax conditions.
> >              (doloop_optimize): Add support for elementwise LoLs.
> >
> >      gcc/testsuite/ChangeLog:
> >
> >              * gcc.target/arm/lob.h: Update framework.
> >              * gcc.target/arm/lob1.c: Likewise.
> >              * gcc.target/arm/lob6.c: Likewise.
> >              * gcc.target/arm/mve/dlstp-compile-asm.c: New test.
> >              * gcc.target/arm/mve/dlstp-int16x8.c: New test.
> >              * gcc.target/arm/mve/dlstp-int32x4.c: New test.
> >              * gcc.target/arm/mve/dlstp-int64x2.c: New test.
> >              * gcc.target/arm/mve/dlstp-int8x16.c: New test.
> >              * gcc.target/arm/mve/dlstp-invalid-asm.c: New test.