[PATCH target/89071] Fix false dependence of scalar operations vrcp/vsqrt/vrsqrt/vrndscale
Uros Bizjak
ubizjak@gmail.com
Fri Oct 25 21:08:00 GMT 2019
On Fri, Oct 25, 2019 at 9:13 PM Hongtao Liu <crazylht@gmail.com> wrote:
>
> Update patch.
>
> On Fri, Oct 25, 2019 at 4:01 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > On Fri, Oct 25, 2019 at 7:55 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > >
> > > On Fri, Oct 25, 2019 at 1:23 PM Hongtao Liu <crazylht@gmail.com> wrote:
> > > >
> > > > On Fri, Oct 25, 2019 at 2:39 AM Uros Bizjak <ubizjak@gmail.com> wrote:
> > > > >
> > > > > On Wed, Oct 23, 2019 at 7:48 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > > > > >
> > > > > > Update patch:
> > > > > > Add m constraint to define_insn (sse_1_round<ssescalaemodesuffix,
> > > > > > *sse_1_round<ssescalaemodesuffix) to fix some unrecognized insn error
> > > > > > when under sse4 but not avx512f.
> > > > >
> > > > > It looks to me that the original insn is incompletely defined. It
> > > > > should use nonimmediate_operand, "m" constraint and <iptr> pointer
> > > > > size modifier. Something like:
> > > > >
> > > > > (define_insn "sse4_1_round<ssescalarmodesuffix>"
> > > > > [(set (match_operand:VF_128 0 "register_operand" "=Yr,*x,x,v")
> > > > > (vec_merge:VF_128
> > > > > (unspec:VF_128
> > > > > [(match_operand:VF_128 2 "nonimmediate_operand" "Yrm,*xm,xm,vm")
> > > > > (match_operand:SI 3 "const_0_to_15_operand" "n,n,n,n")]
> > > > > UNSPEC_ROUND)
> > > > > (match_operand:VF_128 1 "register_operand" "0,0,x,v")
> > > > > (const_int 1)))]
> > > > > "TARGET_SSE4_1"
> > > > > "@
> > > > > round<ssescalarmodesuffix>\t{%3, %2, %0|%0, %<iptr>2, %3}
> > > > > round<ssescalarmodesuffix>\t{%3, %2, %0|%0, %<iptr>2, %3}
> > > > > vround<ssescalarmodesuffix>\t{%3, %2, %1, %0|%0, %1, %<iptr>2, %3}
> > > > > vrndscale<ssescalarmodesuffix>\t{%3, %2, %1, %0|%0, %1, %<iptr>2, %3}"
> > > > >
> > > > > >
> > > > > > Changelog:
> > > > > > gcc/
> > > > > > * config/i386/sse.md: (sse4_1_round<ssescalarmodesuffix>):
> > > > > > Change constraint x to xm
> > > > > > since vround support memory operand.
> > > > > > * (*sse4_1_round<ssescalarmodesuffix>): Ditto.
> > > > > >
> > > > > > Bootstrap and regression test ok.
> > > > > >
> > > > > > On Wed, Oct 23, 2019 at 9:56 AM Hongtao Liu <crazylht@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi uros:
> > > > > > > This patch fixes false dependence of scalar operations
> > > > > > > vrcp/vsqrt/vrsqrt/vrndscale.
> > > > > > > Bootstrap ok, regression test on i386/x86 ok.
> > > > > > >
> > > > > > > It does something like this:
> > > > > > > -----
> > > > > > > For scalar instructions with both xmm operands:
> > > > > > >
> > > > > > > op %xmmN,%xmmQ,%xmmQ ----> op %xmmN, %xmmN, %xmmQ
> > > > > > >
> > > > > > > for scalar instructions with one mem or gpr operand:
> > > > > > >
> > > > > > > op mem/gpr, %xmmQ, %xmmQ
> > > > > > >
> > > > > > > ---> using pass rpad ---->
> > > > > > >
> > > > > > > xorps %xmmN, %xmmN, %xxN
> > > > > > > op mem/gpr, %xmmN, %xmmQ
> > > > > > >
> > > > > > > Performance influence of SPEC2017 fprate which is tested on SKX
> > > > > > >
> > > > > > > 503.bwaves_r -0.03%
> > > > > > > 507.cactuBSSN_r -0.22%
> > > > > > > 508.namd_r -0.02%
> > > > > > > 510.parest_r 0.37%
> > > > > > > 511.povray_r 0.74%
> > > > > > > 519.lbm_r 0.24%
> > > > > > > 521.wrf_r 2.35%
> > > > > > > 526.blender_r 0.71%
> > > > > > > 527.cam4_r 0.65%
> > > > > > > 538.imagick_r 0.95%
> > > > > > > 544.nab_r -0.37
> > > > > > > 549.fotonik3d_r 0.24%
> > > > > > > 554.roms_r 0.90%
> > > > > > > fprate geomean 0.50%
> > > > > > > -----
> > > > > > >
> > > > > > > Changelog
> > > > > > > gcc/
> > > > > > > * config/i386/i386.md (*rcpsf2_sse): Add
> > > > > > > avx_partial_xmm_update, prefer m constraint for TARGET_AVX.
> > > > > > > (*rsqrtsf2_sse): Ditto.
> > > > > > > (*sqrt<mode>2_sse): Ditto.
> > > > > > > (sse4_1_round<mode>2): separate constraint vm, add
> > > > > > > avx_partail_xmm_update, prefer m constraint for TARGET_AVX.
> > > > > > > * config/i386/sse.md (*sse_vmrcpv4sf2"): New define_insn used
> > > > > > > by pass rpad.
> > > > > > > (*<sse>_vmsqrt<mode>2<mask_scalar_name><round_scalar_name>*):
> > > > > > > Ditto.
> > > > > > > (*sse_vmrsqrtv4sf2): Ditto.
> > > > > > > (*avx512f_rndscale<mode><round_saeonly_name>): Ditto.
> > > > > > > (*sse4_1_round<ssescalarmodesuffix>): Ditto.
> > > > > > >
> > > > > > > gcc/testsuite
> > > > > > > * gcc.target/i386/pr87007-4.c: New test.
> > > > > > > * gcc.target/i386/pr87007-5.c: Ditto.
OK.
Thanks,
Uros.
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > BR,
> > > > > > > Hongtao
> > > > >
> > > > > (set (attr "preferred_for_speed")
> > > > > (cond [(eq_attr "alternative" "1")
> > > > > (symbol_ref "TARGET_AVX || !TARGET_SSE_PARTIAL_REG_DEPENDENCY")
> > > > > (eq_attr "alternative" "2")
> > > > > - (symbol_ref "!TARGET_SSE_PARTIAL_REG_DEPENDENCY")
> > > > > + (symbol_ref "TARGET_AVX || !TARGET_SSE_PARTIAL_REG_DEPENDENCY")
> > > > > ]
> > > > > (symbol_ref "true")))])
> > > > >
> > > > > This can be written as:
> > > > >
> > > > > (set (attr "preferred_for_speed")
> > > > > (cond [(match_test "TARGET_AVX")
> > > > > (symbol_ref "true")
> > > > > (eq_attr "alternative" "1,2")
> > > > > (symbol_ref "!TARGET_SSE_PARTIAL_REG_DEPENDENCY")
> > > > > ]
> > > > > (symbol_ref "true")))])
> > > > >
> > > > > Uros.
> > > >
> > > > Yes, after these fixed, i'll upstream to trunk, ok?
> > > Update patch.
> >
> > + (sqrt:<ssescalarmode>
> > + (match_operand:<ssescalarmode> 1 "vector_operand"
> > "xBm,<round_scalar_constraint>")))
> > + (match_operand:VF_128 2 "register_operand" "0,v")
> > + (const_int 1)))]
> >
> > vector_operand and Bm are needed for vector mode operands. This is in
> > effect scalar operand, so nonimmediate_operand and simple "xm" should
> > be used here.
> >
> > +(define_insn "*sse_vmrsqrtv4sf2"
> > + [(set (match_operand:V4SF 0 "register_operand" "=x,x")
> > + (vec_merge:V4SF
> > + (vec_duplicate:V4SF
> > + (unspec:SF [(match_operand:SF 1 "nonimmediate_operand" "xm,xm")]
> > + UNSPEC_RSQRT))
> > + (match_operand:V4SF 2 "register_operand" "0,x")
> > + (const_int 1)))]
> > + "TARGET_SSE"
> > + "@
> > + rsqrtss\t{%1, %0|%0, %k1}
> > + vrsqrtss\t{%1, %2, %0|%0, %2, %k1}"
> >
> > No need for %k modifier. We already have scalar size 4 SFmode operand
> > that will genereate DWORD PTR.
> >
> > +(define_insn "*avx512f_rndscale<mode><round_saeonly_name>"
> > + [(set (match_operand:VF_128 0 "register_operand" "=v")
> > + (vec_merge:VF_128
> > + (vec_duplicate:VF_128
> > + (unspec:<ssescalarmode>
> > + [(match_operand:<ssescalarmode> 2
> > "<round_saeonly_nimm_predicate>" "<round_saeonly_constraint>")
> > + (match_operand:SI 3 "const_0_to_255_operand")]
> > + UNSPEC_ROUND))
> > + (match_operand:VF_128 1 "register_operand" "v")
> > + (const_int 1)))]
> > + "TARGET_AVX512F"
> > + "vrndscale<ssescalarmodesuffix>\t{%3, <round_saeonly_op4>%2, %1,
> > %0|%0, %1, %<iptr>2<round_saeonly_op4>, %3}"
> >
> > There is no need for <iptr> override for scalar mode operands in the
> > above and other new patterns,
> >
> > Looking into sse.md, there is a lot of inconsistencies in existing *vm
> > patterns w.r.t. operand constraints. Unfortunately, these were copied
> > into proposed patterns. One example is existing
> >
> > (define_insn "<sse>_vmsqrt<mode>2<mask_scalar_name><round_scalar_name>"
> > [(set (match_operand:VF_128 0 "register_operand" "=x,v")
> > (vec_merge:VF_128
> > (sqrt:VF_128
> > (match_operand:VF_128 1 "vector_operand"
> > "xBm,<round_scalar_constraint>"))
> > (match_operand:VF_128 2 "register_operand" "0,v")
> > (const_int 1)))]
> > "TARGET_SSE"
> > "@
> > sqrt<ssescalarmodesuffix>\t{%1, %0|%0, %<iptr>1}
> >
> > Due to combine benefits, *vm operands to be merged is described in
> > vector mode. Since the insn operates in scalar mode, there is no need
> > for "vector_operand" and Bm constraint that impose more strict
> > alignment requirements. However, iptr modifier is needed here to
> > override VF_128 vector mode (e.g. V4SFmode) to generate scalar
> > (SFmode, DWORD PTR) memory access prefix.
> >
> > Someone should fix these existing inconsistencies in a follow-up patch.
> >
> > Uros.
>
>
>
> --
> BR,
> Hongtao
More information about the Gcc-patches
mailing list