RE: [PATCH PR96357][GCC][AArch64]: could not split insn UNSPEC_COND_FSUB with AArch64 SVE
> Przemyslaw Wirkus writes: > > Hello maintainers, > > > > Can I backport this patch to GCC 10 please ? > > Sure, that's fine. commit 41d22ec51c4190133a082197e7ff67b4741fc09b Date: Fri Aug 28 11:31:04 2020 +0100 > Thanks, > Richard > > > > > Regards > > Przemyslaw > > > >> Committed with: > >> > >> commit b648814c02eb418aaf27897c480452172ee96303 > >> Date: Fri Aug 28 11:31:04 2020 +0100 > >> > >> Kind regards, > >> Przemyslaw
Re: [PATCH PR96357][GCC][AArch64]: could not split insn UNSPEC_COND_FSUB with AArch64 SVE
Przemyslaw Wirkus writes: > Hello maintainers, > > Can I backport this patch to GCC 10 please ? Sure, that's fine. Thanks, Richard > > Regards > Przemyslaw > >> Committed with: >> >> commit b648814c02eb418aaf27897c480452172ee96303 >> Date: Fri Aug 28 11:31:04 2020 +0100 >> >> Kind regards, >> Przemyslaw
RE: [PATCH PR96357][GCC][AArch64]: could not split insn UNSPEC_COND_FSUB with AArch64 SVE
Hello maintainers, Can I backport this patch to GCC 10 please ? Regards Przemyslaw > Committed with: > > commit b648814c02eb418aaf27897c480452172ee96303 > Date: Fri Aug 28 11:31:04 2020 +0100 > > Kind regards, > Przemyslaw
RE: [PATCH PR96357][GCC][AArch64]: could not split insn UNSPEC_COND_FSUB with AArch64 SVE
> Sorry for the micromanagement, but I think this is easier to read if it flows
> as a
> single paragraph:
[snip...]
> I should have realised this would be the case, sorry, but now that there's
> only
> one rewrite, this should simply be:
>
> "&& reload_completed
>&& register_operand (operands[4], mode)
>&& !rtx_equal_p (operands[0], operands[4]))"
> {
> emit_insn (gen_vcond_mask_ (operands[0], operands[3],
>operands[4], operands[1]));
> operands[4] = operands[3] = operands[0];
> }
Done and done.
> OK with those changes, thanks.
>
> Richard
Committed with:
commit b648814c02eb418aaf27897c480452172ee96303
Date: Fri Aug 28 11:31:04 2020 +0100
Kind regards,
Przemyslaw
Re: [PATCH PR96357][GCC][AArch64]: could not split insn UNSPEC_COND_FSUB with AArch64 SVE
Przemyslaw Wirkus writes:
> @@ -5234,21 +5234,21 @@ (define_insn_and_rewrite "*cond_sub_3_const"
>
> ;; Predicated floating-point subtraction from a constant, merging with an
> ;; independent value.
> -(define_insn_and_rewrite "*cond_sub_any_const"
> +;; The subtraction predicate and the merge predicate are allowed to be
> +;; different.
Sorry for the micromanagement, but I think this is easier to read if it
flows as a single paragraph:
;; Predicated floating-point subtraction from a constant, merging with an
;; independent value. The subtraction predicate and the merge predicate are
;; allowed to be different.
or is written as two separate paragraphs:
;; Predicated floating-point subtraction from a constant, merging with an
;; independent value.
;;
;; The subtraction predicate and the merge predicate are allowed to be
;; different.
Same for the second comment.
> @@ -5271,6 +5271,41 @@ (define_insn_and_rewrite "*cond_sub_any_const"
>[(set_attr "movprfx" "yes")]
> )
>
> +;; Predicated floating-point subtraction from a constant, merging with an
> +;; independent value.
> +;; The subtraction predicate and the merge predicate must be the same.
> +(define_insn_and_rewrite "*cond_sub_strict_const"
> + [(set (match_operand:SVE_FULL_F 0 "register_operand" "=w, w, ?w")
> + (unspec:SVE_FULL_F
> + [(match_operand: 1 "register_operand" "Upl, Upl, Upl")
> +(unspec:SVE_FULL_F
> + [(match_dup 1)
> + (const_int SVE_STRICT_GP)
> + (match_operand:SVE_FULL_F 2 "aarch64_sve_float_arith_immediate")
> + (match_operand:SVE_FULL_F 3 "register_operand" "w, w, w")]
> + UNSPEC_COND_FSUB)
> +(match_operand:SVE_FULL_F 4 "aarch64_simd_reg_or_zero" "Dz, 0, w")]
> + UNSPEC_SEL))]
> + "TARGET_SVE && !rtx_equal_p (operands[3], operands[4])"
> + "@
> + movprfx\t%0., %1/z, %3.\;fsubr\t%0., %1/m,
> %0., #%2
> + movprfx\t%0., %1/m, %3.\;fsubr\t%0., %1/m,
> %0., #%2
> + #"
> + "&& 1"
> + {
> +if (reload_completed
> +&& register_operand (operands[4], mode)
> +&& !rtx_equal_p (operands[0], operands[4]))
> + {
> + emit_insn (gen_vcond_mask_ (operands[0], operands[3],
> + operands[4], operands[1]));
> + operands[4] = operands[3] = operands[0];
> + }
> +else
> + FAIL;
I should have realised this would be the case, sorry, but now that there's
only one rewrite, this should simply be:
"&& reload_completed
&& register_operand (operands[4], mode)
&& !rtx_equal_p (operands[0], operands[4]))"
{
emit_insn (gen_vcond_mask_ (operands[0], operands[3],
operands[4], operands[1]));
operands[4] = operands[3] = operands[0];
}
OK with those changes, thanks.
Richard
RE: [PATCH PR96357][GCC][AArch64]: could not split insn UNSPEC_COND_FSUB with AArch64 SVE
Hi Richard,
Thank you for your comments.
I've attached updated patch with changes reflecting your comments.
Kind regards,
Przemyslaw
> -Original Message-
> From: Richard Sandiford
> Sent: 19 August 2020 11:32
> To: Przemyslaw Wirkus
> Cc: gcc-patches@gcc.gnu.org; Richard Earnshaw
> ; Marcus Shawcroft
> ; Kyrylo Tkachov
> Subject: Re: [PATCH PR96357][GCC][AArch64]: could not split insn
> UNSPEC_COND_FSUB with AArch64 SVE
>
> Przemyslaw Wirkus writes:
> > Hi,
> >
> > Problem is related to that operand 4 (In original pattern
> > *cond_sub_any_const) is no longer the same as operand 1, and so
> > the pattern doesn't match the split condition.
> >
> > Pattern *cond_sub_any_const is being split by this patch into
> > two separate patterns:
> > * Pattern *cond_sub_relaxed_const now matches const_int
> > SVE_RELAXED_GP operand.
> > * Pattern *cond_sub_strict_const now matches const_int
> > SVE_STRICT_GP operand.
> > * Remove aarch64_sve_pred_dominates_p condition from both patterns.
>
> Thanks for doing this.
>
> > @@ -5271,6 +5270,43 @@ (define_insn_and_rewrite
> "*cond_sub_any_const"
> >[(set_attr "movprfx" "yes")]
> > )
> >
> > +;; Predicated floating-point subtraction from a constant, merging
> > +with an ;; independent value.
>
> The previous pattern had the same comment. Maybe add:
>
> The subtraction predicate and the merge predicate are allowed to be
> different.
>
> to the relaxed one and:
>
> The subtraction predicate and the merge predicate must be the same.
>
> to this one.
>
> > +(define_insn_and_rewrite "*cond_sub_strict_const"
> > + [(set (match_operand:SVE_FULL_F 0 "register_operand" "=w, w, ?w")
> > + (unspec:SVE_FULL_F
> > + [(match_operand: 1 "register_operand" "Upl, Upl, Upl")
> > + (unspec:SVE_FULL_F
> > +[(match_dup 1)
> > + (const_int SVE_STRICT_GP)
> > + (match_operand:SVE_FULL_F 2
> "aarch64_sve_float_arith_immediate")
> > + (match_operand:SVE_FULL_F 3 "register_operand" "w, w, w")]
> > +UNSPEC_COND_FSUB)
> > + (match_operand:SVE_FULL_F 4 "aarch64_simd_reg_or_zero" "Dz, 0,
> w")]
> > + UNSPEC_SEL))]
> > + "TARGET_SVE
> > + && !rtx_equal_p (operands[3], operands[4])"
>
> Very minor, but the file generally puts conditions on a single line if
> they'll fit.
> Same for the relaxed version.
>
> > + "@
> > +
> movprfx\t%0., %1/z, %3.\;fsubr\t%0., %1/m, %0.<
> Vetype>, #%2
> > +
> movprfx\t%0., %1/m, %3.\;fsubr\t%0., %1/m, %0.
> , #%2
> > + #"
> > + "&& 1"
> > + {
> > +if (reload_completed
> > +&& register_operand (operands[4], mode)
> > +&& !rtx_equal_p (operands[0], operands[4]))
> > + {
> > + emit_insn (gen_vcond_mask_ (operands[0],
> operands[3],
> > +operands[4], operands[1]));
> > + operands[4] = operands[3] = operands[0];
> > + }
> > +else if (!rtx_equal_p (operands[1], operands[5]))
> > + operands[5] = copy_rtx (operands[1]);
>
> The last two lines are a hold-over from the relaxed version, where there were
> two predicates. There's no operand 5 in this pattern, so we should just
> delete
> the lines.
>
> Thanks,
> Richard
rb13404.patch
Description: rb13404.patch
Re: [PATCH PR96357][GCC][AArch64]: could not split insn UNSPEC_COND_FSUB with AArch64 SVE
Przemyslaw Wirkus writes:
> Hi,
>
> Problem is related to that operand 4 (In original pattern
> *cond_sub_any_const) is no longer the same as operand 1, and so
> the pattern doesn't match the split condition.
>
> Pattern *cond_sub_any_const is being split by this patch into two
> separate patterns:
> * Pattern *cond_sub_relaxed_const now matches const_int
> SVE_RELAXED_GP operand.
> * Pattern *cond_sub_strict_const now matches const_int
> SVE_STRICT_GP operand.
> * Remove aarch64_sve_pred_dominates_p condition from both patterns.
Thanks for doing this.
> @@ -5271,6 +5270,43 @@ (define_insn_and_rewrite "*cond_sub_any_const"
>[(set_attr "movprfx" "yes")]
> )
>
> +;; Predicated floating-point subtraction from a constant, merging with an
> +;; independent value.
The previous pattern had the same comment. Maybe add:
The subtraction predicate and the merge predicate are allowed to be
different.
to the relaxed one and:
The subtraction predicate and the merge predicate must be the same.
to this one.
> +(define_insn_and_rewrite "*cond_sub_strict_const"
> + [(set (match_operand:SVE_FULL_F 0 "register_operand" "=w, w, ?w")
> + (unspec:SVE_FULL_F
> + [(match_operand: 1 "register_operand" "Upl, Upl, Upl")
> +(unspec:SVE_FULL_F
> + [(match_dup 1)
> + (const_int SVE_STRICT_GP)
> + (match_operand:SVE_FULL_F 2 "aarch64_sve_float_arith_immediate")
> + (match_operand:SVE_FULL_F 3 "register_operand" "w, w, w")]
> + UNSPEC_COND_FSUB)
> +(match_operand:SVE_FULL_F 4 "aarch64_simd_reg_or_zero" "Dz, 0, w")]
> + UNSPEC_SEL))]
> + "TARGET_SVE
> + && !rtx_equal_p (operands[3], operands[4])"
Very minor, but the file generally puts conditions on a single line
if they'll fit. Same for the relaxed version.
> + "@
> + movprfx\t%0., %1/z, %3.\;fsubr\t%0., %1/m,
> %0., #%2
> + movprfx\t%0., %1/m, %3.\;fsubr\t%0., %1/m,
> %0., #%2
> + #"
> + "&& 1"
> + {
> +if (reload_completed
> +&& register_operand (operands[4], mode)
> +&& !rtx_equal_p (operands[0], operands[4]))
> + {
> + emit_insn (gen_vcond_mask_ (operands[0], operands[3],
> + operands[4], operands[1]));
> + operands[4] = operands[3] = operands[0];
> + }
> +else if (!rtx_equal_p (operands[1], operands[5]))
> + operands[5] = copy_rtx (operands[1]);
The last two lines are a hold-over from the relaxed version, where there
were two predicates. There's no operand 5 in this pattern, so we should
just delete the lines.
Thanks,
Richard
