Message ID | YkHh2TrgR1+HRG6j@toto.the-meissners.org |
---|---|
State | New |
Headers | show |
Series | Optimize vec_splats of vec_extract, PR target/99293 | expand |
Hi! On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote: > In looking at PR target/99293, I noticed that the code in the insn > vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the > "mtvsrdd" is generated. It should use "mfvsr". I also added a "p9v" isa > attribute for that alternative. s/mfvsr/mtvsr/ But, mtvsrd and mtvsrdd have very different scheduling properties (like, on p10 it is 1 cycle vs. 3 cycles). Also, there are two insn patterns for mtvsrdd, and you are only touching one here. > * config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct > insn type attribute. Add "p9v" isa attribute for generating the > mtvsrdd instruction. This is in vsx.md, instead. > --- a/gcc/config/rs6000/vsx.md > +++ b/gcc/config/rs6000/vsx.md > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg" > "@ > xxpermdi %x0,%x1,%x1,0 > mtvsrdd %x0,%1,%1" > - [(set_attr "type" "vecperm,vecmove")]) > + [(set_attr "type" "vecperm,mtvsr") > + (set_attr "isa" "*,p9v")]) "we" requires "p9v". Please do a full conversion when getting rid of this? That includes requiring TARGET_POWERPC64 for it (not -m64 as its documentation says; the existing implementation of "we" is correct). Segher
On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote: > Hi! > > On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote: > > In looking at PR target/99293, I noticed that the code in the insn > > vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the > > "mtvsrdd" is generated. It should use "mfvsr". I also added a "p9v" isa > > attribute for that alternative. > > s/mfvsr/mtvsr/ > > But, mtvsrd and mtvsrdd have very different scheduling properties (like, > on p10 it is 1 cycle vs. 3 cycles). I must admit, I assumed vecmove was a stand-in for XXMR (i.e. XXLOR). Since its use is used for other cases (mtvsrdd, xxsel/vsel, x{s,v}abs*, x{s,v}nabs*, xsiexpq*), it is probably better to just let things lie, and perhaps relook at it in the GCC 13 time frame. > Also, there are two insn patterns for mtvsrdd, and you are only touching > one here. I think you meant that comment about the third patch (to vsx_extract_<mode>) and not to this patch (to vsx_splat_<mode>_reg) where there are only two alternatives (the first being xxpermdi and the second being mtvsrdd). > > * config/rs6000/rs6000.md (vsx_splat_<mode>_reg): Use the correct > > insn type attribute. Add "p9v" isa attribute for generating the > > mtvsrdd instruction. > > This is in vsx.md, instead. > > > --- a/gcc/config/rs6000/vsx.md > > +++ b/gcc/config/rs6000/vsx.md > > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg" > > "@ > > xxpermdi %x0,%x1,%x1,0 > > mtvsrdd %x0,%1,%1" > > - [(set_attr "type" "vecperm,vecmove")]) > > + [(set_attr "type" "vecperm,mtvsr") > > + (set_attr "isa" "*,p9v")]) > > "we" requires "p9v". Please do a full conversion when getting rid of > this? That includes requiring TARGET_POWERPC64 for it (not -m64 as its > documentation says; the existing implementation of "we" is correct). That is more complex, and likely it should be a GCC 13 thing. Off the top of my head, we would need a new "isa" variant (p9v64) that combines p9v and 64-bit. Originally, I had changed the "we" to "wa", but then I realized it wouldn't work for 32-bit, but I left in setting the alternative.
On Wed, Mar 30, 2022 at 06:41:59PM -0400, Michael Meissner wrote: > On Mon, Mar 28, 2022 at 03:28:39PM -0500, Segher Boessenkool wrote: > > On Mon, Mar 28, 2022 at 12:27:05PM -0400, Michael Meissner wrote: > > > In looking at PR target/99293, I noticed that the code in the insn > > > vsx_splat_<mode>_reg used "vecmove" as the "type" insn attribute when the > > > "mtvsrdd" is generated. It should use "mfvsr". I also added a "p9v" isa > > > attribute for that alternative. > > > > s/mfvsr/mtvsr/ > > > > But, mtvsrd and mtvsrdd have very different scheduling properties (like, > > on p10 it is 1 cycle vs. 3 cycles). > > I must admit, I assumed vecmove was a stand-in for XXMR (i.e. XXLOR). That is "veclogical". I don't think there is any core where this is optimised specially? > Since > its use is used for other cases (mtvsrdd, xxsel/vsel, x{s,v}abs*, x{s,v}nabs*, > xsiexpq*), it is probably better to just let things lie, and perhaps relook at > it in the GCC 13 time frame. Yes, we need to make better categories. The problem is to come up with something that is close enough to what the relevant cores actually do, but in such a way that we do not end up with gazillions of nonsensical separate instruction types. What we care about most for p9 and p10 vector insns is whether something is a 3-cycle op or not. But this differs per core, and in ways that are a little ad-hoc (looked at from far away anyway). For the integer insns we ended up with extra attributes (not just "type"), which is both compact and expressive. We should try to do something like that for vector ops as well. We now have both p9 an p10, with two implementations it should be clearer what a good direction to take will be here. > > Also, there are two insn patterns for mtvsrdd, and you are only touching > > one here. > > I think you meant that comment about the third patch (to vsx_extract_<mode>) > and not to this patch (to vsx_splat_<mode>_reg) where there are only two > alternatives (the first being xxpermdi and the second being mtvsrdd). I mean vsx_concat_<mode> and vsx_splat_<mode>_reg. Both have mtvsrdd (both as alternative 1), but you only update the "type" of the latter here. > > > --- a/gcc/config/rs6000/vsx.md > > > +++ b/gcc/config/rs6000/vsx.md > > > @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg" > > > "@ > > > xxpermdi %x0,%x1,%x1,0 > > > mtvsrdd %x0,%1,%1" > > > - [(set_attr "type" "vecperm,vecmove")]) > > > + [(set_attr "type" "vecperm,mtvsr") > > > + (set_attr "isa" "*,p9v")]) > > > > "we" requires "p9v". Please do a full conversion when getting rid of > > this? That includes requiring TARGET_POWERPC64 for it (not -m64 as its > > documentation says; the existing implementation of "we" is correct). > > That is more complex, and likely it should be a GCC 13 thing. Yes. > Off the top of > my head, we would need a new "isa" variant (p9v64) that combines p9v and > 64-bit. Not at all no. Things that *use* the "isa" attribute can use other attributes as well, if they want. The reason we have "p9v" is because it is so common that a shorthand helps (and *all* p9 vector insns need either it or separate stuff). > Originally, I had changed the "we" to "wa", but then I realized it > wouldn't work for 32-bit, but I left in setting the alternative. Yeah, when I got rid of many of the w* things I left mostly the harder ones for later. Sorry! Segher
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md index ddafbe471dd..ad722cff70f 100644 --- a/gcc/config/rs6000/vsx.md +++ b/gcc/config/rs6000/vsx.md @@ -4580,7 +4580,8 @@ (define_insn "vsx_splat_<mode>_reg" "@ xxpermdi %x0,%x1,%x1,0 mtvsrdd %x0,%1,%1" - [(set_attr "type" "vecperm,vecmove")]) + [(set_attr "type" "vecperm,mtvsr") + (set_attr "isa" "*,p9v")]) (define_insn "vsx_splat_<mode>_mem" [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa")