| Message ID | 37539b6f-8add-4d27-02cf-72f1741e8507@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Fix instruction type | expand |
Hi! On Wed, Sep 09, 2020 at 04:14:37PM -0500, Pat Haugen wrote: > I noticed that some of the VSR<->GPR move instructions are not typed > correctly. This patch fixes those instructions so that the scheduler > treats them with the correct latency. > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -5483,7 +5483,7 @@ (define_insn "lfiwzx" > lxsiwzx %x0,%y1 > mtvsrwz %x0,%1 > xxextractuw %x0,%x1,4" > - [(set_attr "type" "fpload,fpload,mftgpr,vecexts") > + [(set_attr "type" "fpload,fpload,mffgpr,vecexts") > (set_attr "isa" "*,p8v,p8v,p9v")]) Can we rename mftgpr/mffgpr globally? Maybe even as mfvsr and mtvsr, because that is what is actually modeled here? Such names will make it much harder to get confused and use the wrong type, too :-) > diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md > index 54da54c43dc..3a5cf896da8 100644 > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -2885,7 +2885,7 @@ (define_insn "vsx_concat_<mode>" > else > gcc_unreachable (); > } > - [(set_attr "type" "vecperm")]) > + [(set_attr "type" "vecperm,vecmove")]) mtvsrdd is a mtvsr, sorry, mffgpr just the same? It isn't vecmove? > @@ -4440,7 +4440,7 @@ (define_insn "vsx_splat_<mode>_reg" > "@ > xxpermdi %x0,%x1,%x1,0 > mtvsrdd %x0,%1,%1" > - [(set_attr "type" "vecperm")]) > + [(set_attr "type" "vecperm,vecmove")]) Same here. Segher
On 9/9/20 4:41 PM, Segher Boessenkool wrote: > Hi! > > On Wed, Sep 09, 2020 at 04:14:37PM -0500, Pat Haugen wrote: >> I noticed that some of the VSR<->GPR move instructions are not typed >> correctly. This patch fixes those instructions so that the scheduler >> treats them with the correct latency. > >> --- a/gcc/config/rs6000/rs6000.md >> +++ b/gcc/config/rs6000/rs6000.md >> @@ -5483,7 +5483,7 @@ (define_insn "lfiwzx" >> lxsiwzx %x0,%y1 >> mtvsrwz %x0,%1 >> xxextractuw %x0,%x1,4" >> - [(set_attr "type" "fpload,fpload,mftgpr,vecexts") >> + [(set_attr "type" "fpload,fpload,mffgpr,vecexts") >> (set_attr "isa" "*,p8v,p8v,p9v")]) > > Can we rename mftgpr/mffgpr globally? Maybe even as mfvsr and mtvsr, > because that is what is actually modeled here? Such names will make it > much harder to get confused and use the wrong type, too :-) > Those types were originally created for the mffgpr/mftgpr Power6 instructions. But since it appears we no longer generate those insns I totally agree with doing a global change as you suggest to make things clearer. Would you like that as a separate patch or is it fine to include in this one? >> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md >> index 54da54c43dc..3a5cf896da8 100644 >> --- a/gcc/config/rs6000/vsx.md >> +++ b/gcc/config/rs6000/vsx.md >> @@ -2885,7 +2885,7 @@ (define_insn "vsx_concat_<mode>" >> else >> gcc_unreachable (); >> } >> - [(set_attr "type" "vecperm")]) >> + [(set_attr "type" "vecperm,vecmove")]) > > mtvsrdd is a mtvsr, sorry, mffgpr just the same? It isn't vecmove? > >> @@ -4440,7 +4440,7 @@ (define_insn "vsx_splat_<mode>_reg" >> "@ >> xxpermdi %x0,%x1,%x1,0 >> mtvsrdd %x0,%1,%1" >> - [(set_attr "type" "vecperm")]) >> + [(set_attr "type" "vecperm,vecmove")]) > > Same here. mtvsrdd dispatches as a vector op, so requires a super-slice. As opposed to the others which just require a single execution slice for Power9. -Pat
On Wed, Sep 09, 2020 at 05:30:33PM -0500, Pat Haugen wrote: > On 9/9/20 4:41 PM, Segher Boessenkool wrote: > > On Wed, Sep 09, 2020 at 04:14:37PM -0500, Pat Haugen wrote: > > Can we rename mftgpr/mffgpr globally? Maybe even as mfvsr and mtvsr, > > because that is what is actually modeled here? Such names will make it > > much harder to get confused and use the wrong type, too :-) > > Those types were originally created for the mffgpr/mftgpr Power6 > instructions. But since it appears we no longer generate those insns I Yes, I know ;-) > totally agree with doing a global change as you suggest to make things > clearer. Would you like that as a separate patch or is it fine to > include in this one? That will be pretty big and mechanic, so separate please. Either before or after this one. power6.md still uses this attribute to describe the p6-specific insns scheduling. Not sure what to do with that? Remove it, or if we leave it, add a comment? > >> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md > >> index 54da54c43dc..3a5cf896da8 100644 > >> --- a/gcc/config/rs6000/vsx.md > >> +++ b/gcc/config/rs6000/vsx.md > >> @@ -2885,7 +2885,7 @@ (define_insn "vsx_concat_<mode>" > >> else > >> gcc_unreachable (); > >> } > >> - [(set_attr "type" "vecperm")]) > >> + [(set_attr "type" "vecperm,vecmove")]) > > > > mtvsrdd is a mtvsr, sorry, mffgpr just the same? It isn't vecmove? > > > >> @@ -4440,7 +4440,7 @@ (define_insn "vsx_splat_<mode>_reg" > >> "@ > >> xxpermdi %x0,%x1,%x1,0 > >> mtvsrdd %x0,%1,%1" > >> - [(set_attr "type" "vecperm")]) > >> + [(set_attr "type" "vecperm,vecmove")]) > > > > Same here. > > mtvsrdd dispatches as a vector op, so requires a super-slice. As opposed > to the others which just require a single execution slice for Power9. Ah in that sense. Okay for trunk then (and backports if we want those). Thanks! Segher
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index 43b620ae1c0..f902c864c26 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -5483,7 +5483,7 @@ (define_insn "lfiwzx" lxsiwzx %x0,%y1 mtvsrwz %x0,%1 xxextractuw %x0,%x1,4" - [(set_attr "type" "fpload,fpload,mftgpr,vecexts") + [(set_attr "type" "fpload,fpload,mffgpr,vecexts") (set_attr "isa" "*,p8v,p8v,p9v")]) (define_insn_and_split "floatunssi<mode>2_lfiwzx" @@ -7634,7 +7634,7 @@ (define_insn_and_split "movsf_from_si" *, 12, *, *") (set_attr "type" "load, fpload, fpload, fpload, store, fpstore, - fpstore, vecfloat, mffgpr, *") + fpstore, vecfloat, mftgpr, *") (set_attr "isa" "*, *, p9v, p8v, *, *, p8v, p8v, p8v, *")]) @@ -8711,7 +8711,7 @@ (define_insn "p8_mtvsrwz" UNSPEC_P8V_MTVSRWZ))] "!TARGET_POWERPC64 && TARGET_DIRECT_MOVE" "mtvsrwz %x0,%1" - [(set_attr "type" "mftgpr")]) + [(set_attr "type" "mffgpr")]) (define_insn_and_split "reload_fpr_from_gpr<mode>" [(set (match_operand:FMOVE64X 0 "register_operand" "=d") @@ -8810,7 +8810,7 @@ (define_insn "p8_mtvsrd_sf" UNSPEC_P8V_MTVSRD))] "TARGET_POWERPC64 && TARGET_DIRECT_MOVE" "mtvsrd %x0,%1" - [(set_attr "type" "mftgpr")]) + [(set_attr "type" "mffgpr")]) (define_insn_and_split "reload_vsx_from_gprsf" [(set (match_operand:SF 0 "register_operand" "=wa") diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md index 54da54c43dc..3a5cf896da8 100644 --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -2885,7 +2885,7 @@ (define_insn "vsx_concat_<mode>" else gcc_unreachable (); } - [(set_attr "type" "vecperm")]) + [(set_attr "type" "vecperm,vecmove")]) ;; Combiner patterns to allow creating XXPERMDI's to access either double ;; word element in a vector register. @@ -4440,7 +4440,7 @@ (define_insn "vsx_splat_<mode>_reg" "@ xxpermdi %x0,%x1,%x1,0 mtvsrdd %x0,%1,%1" - [(set_attr "type" "vecperm")]) + [(set_attr "type" "vecperm,vecmove")]) (define_insn "vsx_splat_<mode>_mem" [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa") @@ -4493,7 +4493,7 @@ (define_insn_and_split "vsx_splat_v4sf" (unspec:V4SF [(match_dup 0) (const_int 0)] UNSPEC_VSX_XXSPLTW))] "" - [(set_attr "type" "vecload,vecperm,mftgpr") + [(set_attr "type" "vecload,vecperm,vecperm") (set_attr "length" "*,8,*") (set_attr "isa" "*,p8v,*")])