On Fri, 29 Aug 2025 at 16:01, Richard Earnshaw <[email protected]> wrote: > > On 29/08/2025 14:57, Christophe Lyon wrote: > > On Fri, 29 Aug 2025 at 15:50, Christophe Lyon > > <[email protected]> wrote: > >> > >> On Fri, 29 Aug 2025 at 15:35, Richard Earnshaw <[email protected]> > >> wrote: > >>> > >>> On 27/08/2025 15:45, Christophe Lyon wrote: > >>>> The thumb2_asrl, thumb2_lsll and thumb2_lsrl patterns were incorrecly > >>>> using (match_dup 0) for the first argument of the shift operator. > >>>> > >>>> This patch replaces that with (match_operand:DI 1 > >>>> arm_general_register_operandarm_general_register_operand "0") and > >>>> fixes the related expanders in arm.md to use that additional argument > >>>> and get rid of the copy of operands[1] to operands[0]. > >>>> > >>>> Finally, since these patterns are MVE-only, rename them into mve_XXX > >>>> and move them to mve.md. > >>>> > >>>> gcc/ChangeLog: > >>>> > >>>> * config/arm/thumb2.md (thumb2_asrl, thumb2_lsll, thumb2_lsrl): > >>>> Move to ... > >>>> * config/arm/mve.md (mve_asrl, mve_lsll, mve_lsrl): ... here. Use > >>>> match_operand instead of match_dup. > >>>> * config/arm/arm.md (ashldi3, ashrdi3, lshrdi3): Remove useless > >>>> copy. Update for new prototype. > >>>> --- > >>>> gcc/config/arm/arm.md | 15 +++------------ > >>>> gcc/config/arm/mve.md | 25 +++++++++++++++++++++++++ > >>>> gcc/config/arm/thumb2.md | 24 ------------------------ > >>>> 3 files changed, 28 insertions(+), 36 deletions(-) > >>>> > >>>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md > >>>> index 537a3e26a45..e0d2a45669e 100644 > >>>> --- a/gcc/config/arm/arm.md > >>>> +++ b/gcc/config/arm/arm.md > >>>> @@ -4588,10 +4588,7 @@ (define_expand "ashldi3" > >>>> if (arm_reg_or_long_shift_imm (operands[2], GET_MODE > >>>> (operands[2])) > >>>> && (REG_P (operands[2]) || INTVAL(operands[2]) != 32)) > >>>> { > >>>> - if (!reg_overlap_mentioned_p(operands[0], operands[1])) > >>>> - emit_insn (gen_movdi (operands[0], operands[1])); > >>>> - > >>>> - emit_insn (gen_thumb2_lsll (operands[0], operands[2])); > >>>> + emit_insn (gen_mve_lsll (operands[0], operands[1], operands[2])); > >>>> DONE; > >>>> } > >>>> } > >>>> @@ -4627,10 +4624,7 @@ (define_expand "ashrdi3" > >>>> if (TARGET_HAVE_MVE && !BYTES_BIG_ENDIAN > >>>> && arm_reg_or_long_shift_imm (operands[2], GET_MODE > >>>> (operands[2]))) > >>>> { > >>>> - if (!reg_overlap_mentioned_p(operands[0], operands[1])) > >>>> - emit_insn (gen_movdi (operands[0], operands[1])); > >>>> - > >>>> - emit_insn (gen_thumb2_asrl (operands[0], operands[2])); > >>>> + emit_insn (gen_mve_asrl (operands[0], operands[1], operands[2])); > >>>> DONE; > >>>> } > >>>> > >>>> @@ -4662,10 +4656,7 @@ (define_expand "lshrdi3" > >>>> if (TARGET_HAVE_MVE && !BYTES_BIG_ENDIAN > >>>> && long_shift_imm (operands[2], GET_MODE (operands[2]))) > >>>> { > >>>> - if (!reg_overlap_mentioned_p(operands[0], operands[1])) > >>>> - emit_insn (gen_movdi (operands[0], operands[1])); > >>>> - > >>>> - emit_insn (gen_thumb2_lsrl (operands[0], operands[2])); > >>>> + emit_insn (gen_mve_lsrl (operands[0], operands[1], operands[2])); > >>>> DONE; > >>>> } > >>>> > >>>> diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md > >>>> index 70b1c77fdce..4636ff356f0 100644 > >>>> --- a/gcc/config/arm/mve.md > >>>> +++ b/gcc/config/arm/mve.md > >>>> @@ -4703,3 +4703,28 @@ (define_insn "dlstp<dlstp_elemsize>_insn" > >>>> "TARGET_HAVE_MVE" > >>>> "dlstp.<dlstp_elemsize>\t%|lr, %0" > >>>> [(set_attr "type" "mve_misc")]) > >>>> + > >>>> +;; Scalar shifts > >>>> +(define_insn "mve_asrl" > >>>> + [(set (match_operand:DI 0 "arm_general_register_operand" "=r") > >>>> + (ashiftrt:DI (match_operand:DI 1 "arm_general_register_operand" > >>>> "0") > >>>> + (match_operand:SI 2 "arm_reg_or_long_shift_imm" > >>>> "rPg")))] > >>>> + "TARGET_HAVE_MVE" > >>>> + "asrl%?\\t%Q0, %R1, %2" > >>>> + [(set_attr "predicable" "yes")]) > >>>> + > >>>> +(define_insn "mve_lsll" > >>>> + [(set (match_operand:DI 0 "arm_general_register_operand" "=r") > >>>> + (ashift:DI (match_operand:DI 1 "arm_general_register_operand" "0") > >>>> + (match_operand:SI 2 "arm_reg_or_long_shift_imm" > >>>> "rPg")))] > >>>> + "TARGET_HAVE_MVE" > >>>> + "lsll%?\\t%Q0, %R1, %2" > >>>> + [(set_attr "predicable" "yes")]) > >>>> + > >>>> +(define_insn "mve_lsrl" > >>>> + [(set (match_operand:DI 0 "arm_general_register_operand" "=r") > >>>> + (lshiftrt:DI (match_operand:DI 1 "arm_general_register_operand" > >>>> "0") > >>>> + (match_operand:SI 2 "long_shift_imm" "Pg")))] > >>>> + "TARGET_HAVE_MVE" > >>>> + "lsrl%?\\t%Q0, %R1, %2" > >>>> + [(set_attr "predicable" "yes")]) > >>>> diff --git a/gcc/config/arm/thumb2.md b/gcc/config/arm/thumb2.md > >>>> index 2c2026b1e74..40c0e052946 100644 > >>>> --- a/gcc/config/arm/thumb2.md > >>>> +++ b/gcc/config/arm/thumb2.md > >>>> @@ -1733,30 +1733,6 @@ (define_insn "*clear_multiple" > >>>> [(set_attr "predicable" "yes")] > >>>> ) > >>>> > >>>> -(define_insn "thumb2_asrl" > >>>> - [(set (match_operand:DI 0 "arm_general_register_operand" "+r") > >>>> - (ashiftrt:DI (match_dup 0) > >>>> - (match_operand:SI 1 "arm_reg_or_long_shift_imm" > >>>> "rPg")))] > >>>> - "TARGET_HAVE_MVE" > >>>> - "asrl%?\\t%Q0, %R0, %1" > >>>> - [(set_attr "predicable" "yes")]) > >>>> - > >>>> -(define_insn "thumb2_lsll" > >>>> - [(set (match_operand:DI 0 "arm_general_register_operand" "+r") > >>>> - (ashift:DI (match_dup 0) > >>>> - (match_operand:SI 1 "arm_reg_or_long_shift_imm" > >>>> "rPg")))] > >>>> - "TARGET_HAVE_MVE" > >>>> - "lsll%?\\t%Q0, %R0, %1" > >>>> - [(set_attr "predicable" "yes")]) > >>>> - > >>>> -(define_insn "thumb2_lsrl" > >>>> - [(set (match_operand:DI 0 "arm_general_register_operand" "+r") > >>>> - (lshiftrt:DI (match_dup 0) > >>>> - (match_operand:SI 1 "long_shift_imm" "Pg")))] > >>>> - "TARGET_HAVE_MVE" > >>>> - "lsrl%?\\t%Q0, %R0, %1" > >>>> - [(set_attr "predicable" "yes")]) > >>>> - > >>>> ;; Originally expanded by 'doloop_end'. > >>>> (define_insn "*doloop_end_internal" > >>>> [(set (pc) > >>> > >>> This is OK, but I'm not really sure it belongs in this patch set. If we > >>> needed to back out (or backport) the other bits, this would need to be > >>> treated separately. > >>> > >> > >> Right... I just happened to notice the problem while working on patches 3 > >> and 4. > >> I can certainly move this one before the series. > >> > > > > Sorry I forgot to ask: is it OK to backport the series to gcc-15 ? > > Is there a PR for it that would justify that? We normally only > back-port fixes for crashes or wrong-code. > Not AFAIK.
The new implementation of lsll and asrl fixes the problem reported by Keith in https://gcc.gnu.org/pipermail/gcc-patches/2025-August/692431.html I can create a PR though. Christophe > R. > > > > > Thanks, > > > > Christophe > > > >> Thanks > >> > >> Christophe > >> > >>> R. >
