| Message ID | 20240130143132.9575-2-andre.simoesdiasvieira@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | vect, aarch64: Add SVE support for simdclones | expand |
On Tue, 30 Jan 2024, Andre Vieira wrote: > > This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the > target can reject a simd_clone based on the vector mode it is using. > This is needed because for VLS SVE vectorization the vectorizer accepts > Advanced SIMD simd clones when vectorizing using SVE types because the simdlens > might match. This will cause type errors later on. > > Other targets do not currently need to use this argument. Can you instead pass down the mode? > gcc/ChangeLog: > > * target.def (TARGET_SIMD_CLONE_USABLE): Add argument. > * tree-vect-stmts.cc (vectorizable_simd_clone_call): Pass stmt_info to > call TARGET_SIMD_CLONE_USABLE. > * config/aarch64/aarch64.cc (aarch64_simd_clone_usable): Add argument > and use it to reject the use of SVE simd clones with Advanced SIMD > modes. > * config/gcn/gcn.cc (gcn_simd_clone_usable): Add unused argument. > * config/i386/i386.cc (ix86_simd_clone_usable): Likewise. > >
On Wed, 31 Jan 2024, Richard Biener wrote: > On Tue, 30 Jan 2024, Andre Vieira wrote: > > > > > This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the > > target can reject a simd_clone based on the vector mode it is using. > > This is needed because for VLS SVE vectorization the vectorizer accepts > > Advanced SIMD simd clones when vectorizing using SVE types because the simdlens > > might match. This will cause type errors later on. > > > > Other targets do not currently need to use this argument. > > Can you instead pass down the mode? Thinking about that again the cgraph_simd_clone info in the clone should have sufficient information to disambiguate. If it doesn't then we should amend it. Richard.
On 31/01/2024 12:13, Richard Biener wrote: > On Wed, 31 Jan 2024, Richard Biener wrote: > >> On Tue, 30 Jan 2024, Andre Vieira wrote: >> >>> >>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the >>> target can reject a simd_clone based on the vector mode it is using. >>> This is needed because for VLS SVE vectorization the vectorizer accepts >>> Advanced SIMD simd clones when vectorizing using SVE types because the simdlens >>> might match. This will cause type errors later on. >>> >>> Other targets do not currently need to use this argument. >> >> Can you instead pass down the mode? > > Thinking about that again the cgraph_simd_clone info in the clone > should have sufficient information to disambiguate. If it doesn't > then we should amend it. > > Richard. Hi Richard, Thanks for the review, I don't think cgraph_simd_clone_info is the right place to pass down this information, since this is information about the caller rather than the simdclone itself. What we are trying to achieve here is making the vectorizer being able to accept or reject simdclones based on the ISA we are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we use modes, I am also not sure that's ideal but it is what we currently use. So to answer your earlier question, yes I can also pass down mode if that's preferable. Regards, Andre
On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > On 31/01/2024 12:13, Richard Biener wrote: > > On Wed, 31 Jan 2024, Richard Biener wrote: > > > >> On Tue, 30 Jan 2024, Andre Vieira wrote: > >> > >>> > >>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the > >>> target can reject a simd_clone based on the vector mode it is using. > >>> This is needed because for VLS SVE vectorization the vectorizer accepts > >>> Advanced SIMD simd clones when vectorizing using SVE types because the > >>> simdlens > >>> might match. This will cause type errors later on. > >>> > >>> Other targets do not currently need to use this argument. > >> > >> Can you instead pass down the mode? > > > > Thinking about that again the cgraph_simd_clone info in the clone > > should have sufficient information to disambiguate. If it doesn't > > then we should amend it. > > > > Richard. > > Hi Richard, > > Thanks for the review, I don't think cgraph_simd_clone_info is the right place > to pass down this information, since this is information about the caller > rather than the simdclone itself. What we are trying to achieve here is making > the vectorizer being able to accept or reject simdclones based on the ISA we > are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we use > modes, I am also not sure that's ideal but it is what we currently use. So to > answer your earlier question, yes I can also pass down mode if that's > preferable. Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere whether that's POLY or constant. I wonder how aarch64_sve_mode_p comes into play here which in the end classifies VLS SVE modes as non-SVE? > Regards, > Andre >
On Wed, 31 Jan 2024, Richard Biener wrote: > On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > > > > > On 31/01/2024 12:13, Richard Biener wrote: > > > On Wed, 31 Jan 2024, Richard Biener wrote: > > > > > >> On Tue, 30 Jan 2024, Andre Vieira wrote: > > >> > > >>> > > >>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the > > >>> target can reject a simd_clone based on the vector mode it is using. > > >>> This is needed because for VLS SVE vectorization the vectorizer accepts > > >>> Advanced SIMD simd clones when vectorizing using SVE types because the > > >>> simdlens > > >>> might match. This will cause type errors later on. > > >>> > > >>> Other targets do not currently need to use this argument. > > >> > > >> Can you instead pass down the mode? > > > > > > Thinking about that again the cgraph_simd_clone info in the clone > > > should have sufficient information to disambiguate. If it doesn't > > > then we should amend it. > > > > > > Richard. > > > > Hi Richard, > > > > Thanks for the review, I don't think cgraph_simd_clone_info is the right place > > to pass down this information, since this is information about the caller > > rather than the simdclone itself. What we are trying to achieve here is making > > the vectorizer being able to accept or reject simdclones based on the ISA we > > are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we use > > modes, I am also not sure that's ideal but it is what we currently use. So to > > answer your earlier question, yes I can also pass down mode if that's > > preferable. > > Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere > whether that's POLY or constant. I wonder how aarch64_sve_mode_p > comes into play here which in the end classifies VLS SVE modes as > non-SVE? Maybe it's just a bit non-obvious as you key on mangling: static int -aarch64_simd_clone_usable (struct cgraph_node *node) +aarch64_simd_clone_usable (struct cgraph_node *node, stmt_vec_info stmt_vinfo) { switch (node->simdclone->vecsize_mangle) { case 'n': if (!TARGET_SIMD) return -1; + if (STMT_VINFO_VECTYPE (stmt_vinfo) + && aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)))) + return -1; ? What does 'n' mean? It's documented as /* The mangling character for a given vector size. This is used to determine the ISA mangling bit as specified in the Intel Vector ABI. */ unsigned char vecsize_mangle; which is slightly misleading.
On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > On 31/01/2024 13:58, Richard Biener wrote: > > On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > >> > >> > >> On 31/01/2024 12:13, Richard Biener wrote: > >>> On Wed, 31 Jan 2024, Richard Biener wrote: > >>> > >>>> On Tue, 30 Jan 2024, Andre Vieira wrote: > >>>> > >>>>> > >>>>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure > >>>>> the > >>>>> target can reject a simd_clone based on the vector mode it is using. > >>>>> This is needed because for VLS SVE vectorization the vectorizer accepts > >>>>> Advanced SIMD simd clones when vectorizing using SVE types because the > >>>>> simdlens > >>>>> might match. This will cause type errors later on. > >>>>> > >>>>> Other targets do not currently need to use this argument. > >>>> > >>>> Can you instead pass down the mode? > >>> > >>> Thinking about that again the cgraph_simd_clone info in the clone > >>> should have sufficient information to disambiguate. If it doesn't > >>> then we should amend it. > >>> > >>> Richard. > >> > >> Hi Richard, > >> > >> Thanks for the review, I don't think cgraph_simd_clone_info is the right > >> place > >> to pass down this information, since this is information about the caller > >> rather than the simdclone itself. What we are trying to achieve here is > >> making > >> the vectorizer being able to accept or reject simdclones based on the ISA > >> we > >> are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we > >> use > >> modes, I am also not sure that's ideal but it is what we currently use. So > >> to > >> answer your earlier question, yes I can also pass down mode if that's > >> preferable. > > > > Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere > > whether that's POLY or constant. I wonder how aarch64_sve_mode_p > > comes into play here which in the end classifies VLS SVE modes as > > non-SVE? > > > > Using -msve-vector-bits=128 > (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) > $4 = E_VNx4SImode > (gdb) p TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo)) > $5 = (tree) 0xfffff741c1b0 > (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) > 128 > (gdb) p aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo))) > $5 = true > > and for reference without vls codegen: > (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) > $1 = E_VNx4SImode > (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) > POLY_INT_CST [128, 128] > > Having said that I believe that the USABLE targethook implementation for > aarch64 should also block other uses, like an Advanced SIMD mode being used as > input for a SVE VLS SIMDCLONE. The reason being that for instance 'half' > registers like VNx2SI are packed differently from V2SI. > > We could teach the vectorizer to support these of course, but that requires > more work and is not extremely useful just yet. I'll add the extra check that > to the patch once we agree on how to pass down the information we need. Happy > to use either mode, or stmt_vec_info and extract the mode from it like it does > now. As said, please pass down 'mode'. But I wonder how to document it, which mode is that supposed to be? Any of result or any argument mode that happens to be a vector? I think that we might be able to mix Advanced SIMD modes and SVE modes with -msve-vector-bits=128 in the same loop? Are the simd clones you don't want to use with -msve-vector-bits=128 having constant simdlen? If so why do you generate them in the first place? That said, I wonder how we end up mixing things up in the first place. Richard.
On 31/01/2024 13:58, Richard Biener wrote: > On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > >> >> >> On 31/01/2024 12:13, Richard Biener wrote: >>> On Wed, 31 Jan 2024, Richard Biener wrote: >>> >>>> On Tue, 30 Jan 2024, Andre Vieira wrote: >>>> >>>>> >>>>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the >>>>> target can reject a simd_clone based on the vector mode it is using. >>>>> This is needed because for VLS SVE vectorization the vectorizer accepts >>>>> Advanced SIMD simd clones when vectorizing using SVE types because the >>>>> simdlens >>>>> might match. This will cause type errors later on. >>>>> >>>>> Other targets do not currently need to use this argument. >>>> >>>> Can you instead pass down the mode? >>> >>> Thinking about that again the cgraph_simd_clone info in the clone >>> should have sufficient information to disambiguate. If it doesn't >>> then we should amend it. >>> >>> Richard. >> >> Hi Richard, >> >> Thanks for the review, I don't think cgraph_simd_clone_info is the right place >> to pass down this information, since this is information about the caller >> rather than the simdclone itself. What we are trying to achieve here is making >> the vectorizer being able to accept or reject simdclones based on the ISA we >> are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we use >> modes, I am also not sure that's ideal but it is what we currently use. So to >> answer your earlier question, yes I can also pass down mode if that's >> preferable. > > Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere > whether that's POLY or constant. I wonder how aarch64_sve_mode_p > comes into play here which in the end classifies VLS SVE modes as > non-SVE? > Using -msve-vector-bits=128 (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) $4 = E_VNx4SImode (gdb) p TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo)) $5 = (tree) 0xfffff741c1b0 (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) 128 (gdb) p aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo))) $5 = true and for reference without vls codegen: (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) $1 = E_VNx4SImode (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) POLY_INT_CST [128, 128] Having said that I believe that the USABLE targethook implementation for aarch64 should also block other uses, like an Advanced SIMD mode being used as input for a SVE VLS SIMDCLONE. The reason being that for instance 'half' registers like VNx2SI are packed differently from V2SI. We could teach the vectorizer to support these of course, but that requires more work and is not extremely useful just yet. I'll add the extra check that to the patch once we agree on how to pass down the information we need. Happy to use either mode, or stmt_vec_info and extract the mode from it like it does now. >> Regards, >> Andre >> >
On 31/01/2024 14:03, Richard Biener wrote: > On Wed, 31 Jan 2024, Richard Biener wrote: > >> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: >> >>> >>> >>> On 31/01/2024 12:13, Richard Biener wrote: >>>> On Wed, 31 Jan 2024, Richard Biener wrote: >>>> >>>>> On Tue, 30 Jan 2024, Andre Vieira wrote: >>>>> >>>>>> >>>>>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the >>>>>> target can reject a simd_clone based on the vector mode it is using. >>>>>> This is needed because for VLS SVE vectorization the vectorizer accepts >>>>>> Advanced SIMD simd clones when vectorizing using SVE types because the >>>>>> simdlens >>>>>> might match. This will cause type errors later on. >>>>>> >>>>>> Other targets do not currently need to use this argument. >>>>> >>>>> Can you instead pass down the mode? >>>> >>>> Thinking about that again the cgraph_simd_clone info in the clone >>>> should have sufficient information to disambiguate. If it doesn't >>>> then we should amend it. >>>> >>>> Richard. >>> >>> Hi Richard, >>> >>> Thanks for the review, I don't think cgraph_simd_clone_info is the right place >>> to pass down this information, since this is information about the caller >>> rather than the simdclone itself. What we are trying to achieve here is making >>> the vectorizer being able to accept or reject simdclones based on the ISA we >>> are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we use >>> modes, I am also not sure that's ideal but it is what we currently use. So to >>> answer your earlier question, yes I can also pass down mode if that's >>> preferable. >> >> Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere >> whether that's POLY or constant. I wonder how aarch64_sve_mode_p >> comes into play here which in the end classifies VLS SVE modes as >> non-SVE? > > Maybe it's just a bit non-obvious as you key on mangling: > > static int > -aarch64_simd_clone_usable (struct cgraph_node *node) > +aarch64_simd_clone_usable (struct cgraph_node *node, stmt_vec_info > stmt_vinfo) > { > switch (node->simdclone->vecsize_mangle) > { > case 'n': > if (!TARGET_SIMD) > return -1; > + if (STMT_VINFO_VECTYPE (stmt_vinfo) > + && aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE > (stmt_vinfo)))) > + return -1; > > ? What does 'n' mean? It's documented as > > /* The mangling character for a given vector size. This is used > to determine the ISA mangling bit as specified in the Intel > Vector ABI. */ > unsigned char vecsize_mangle; I'll update the comment, but yeh 'n' is for Advanced SIMD, 's' is for SVE. > > which is slightly misleading.
On 31/01/2024 14:35, Richard Biener wrote: > On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > >> >> >> On 31/01/2024 13:58, Richard Biener wrote: >>> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: >>> >>>> >>>> >>>> On 31/01/2024 12:13, Richard Biener wrote: >>>>> On Wed, 31 Jan 2024, Richard Biener wrote: >>>>> >>>>>> On Tue, 30 Jan 2024, Andre Vieira wrote: >>>>>> >>>>>>> >>>>>>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure >>>>>>> the >>>>>>> target can reject a simd_clone based on the vector mode it is using. >>>>>>> This is needed because for VLS SVE vectorization the vectorizer accepts >>>>>>> Advanced SIMD simd clones when vectorizing using SVE types because the >>>>>>> simdlens >>>>>>> might match. This will cause type errors later on. >>>>>>> >>>>>>> Other targets do not currently need to use this argument. >>>>>> >>>>>> Can you instead pass down the mode? >>>>> >>>>> Thinking about that again the cgraph_simd_clone info in the clone >>>>> should have sufficient information to disambiguate. If it doesn't >>>>> then we should amend it. >>>>> >>>>> Richard. >>>> >>>> Hi Richard, >>>> >>>> Thanks for the review, I don't think cgraph_simd_clone_info is the right >>>> place >>>> to pass down this information, since this is information about the caller >>>> rather than the simdclone itself. What we are trying to achieve here is >>>> making >>>> the vectorizer being able to accept or reject simdclones based on the ISA >>>> we >>>> are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we >>>> use >>>> modes, I am also not sure that's ideal but it is what we currently use. So >>>> to >>>> answer your earlier question, yes I can also pass down mode if that's >>>> preferable. >>> >>> Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere >>> whether that's POLY or constant. I wonder how aarch64_sve_mode_p >>> comes into play here which in the end classifies VLS SVE modes as >>> non-SVE? >>> >> >> Using -msve-vector-bits=128 >> (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) >> $4 = E_VNx4SImode >> (gdb) p TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo)) >> $5 = (tree) 0xfffff741c1b0 >> (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) >> 128 >> (gdb) p aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo))) >> $5 = true >> >> and for reference without vls codegen: >> (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) >> $1 = E_VNx4SImode >> (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) >> POLY_INT_CST [128, 128] >> >> Having said that I believe that the USABLE targethook implementation for >> aarch64 should also block other uses, like an Advanced SIMD mode being used as >> input for a SVE VLS SIMDCLONE. The reason being that for instance 'half' >> registers like VNx2SI are packed differently from V2SI. >> >> We could teach the vectorizer to support these of course, but that requires >> more work and is not extremely useful just yet. I'll add the extra check that >> to the patch once we agree on how to pass down the information we need. Happy >> to use either mode, or stmt_vec_info and extract the mode from it like it does >> now. > > As said, please pass down 'mode'. But I wonder how to document it, > which mode is that supposed to be? Any of result or any argument > mode that happens to be a vector? I think that we might be able > to mix Advanced SIMD modes and SVE modes with -msve-vector-bits=128 > in the same loop? > > Are the simd clones you don't want to use with -msve-vector-bits=128 > having constant simdlen? If so why do you generate them in the first > place? So this is where things get a bit confusing and I will write up some text for these cases to put in our ABI document (currently in Beta and in need of some tlc). Our intended behaviour is for a 'declare simd' without a simdlen to generate simdclones for: * Advanced SIMD 128 and 64-bit vectors, where possible (we don't allow for simdlen 1, Tamar fixed that in gcc recently), * SVE VLA vectors. Let me illustrate this with an example: __attribute__ ((simd (notinbranch), const)) float cosf(float); Should tell the compiler the following simd clones are available: __ZGVnN4v_cosf 128-bit 4x4 float Advanced SIMD clone __ZGVnN2v_cosf 64-bit 4x2 float Advanced SIMD clone __ZGVsMxv_cosf [128, 128]-bit 4x4xN SVE SIMD clone [To save you looking into the abi let me break this down, _ZGV is prefix, then 'n' or 's' picks between Advanced SIMD and SVE, 'N' or 'M' picks between Not Masked and Masked (SVE is always masked even if we ask for notinbranch), then a digit or 'x' picks between Vector Length or VLA, and after that you get a letter per argument, where v = vector mapped] Regardless of -msve-vector-bits, however, the vectorizer (and any other part of the compiler) may assume that the VL of the VLA SVE clone is that specified by -msve-vector-bits, which if the clone is written in a VLA way will still work. If the attribute is used with a function definition rather than declaration, so: __attribute__ ((simd (notinbranch), const)) float fn0(float a) { return a + 1.0f; } the compiler should again generate the three simd clones: __ZGVnN4v_fn0 128-bit 4x4 float Advanced SIMD clone __ZGVnN2v_fn0 64-bit 4x2 float Advanced SIMD clone __ZGVsMxv_fn0 [128, 128]-bit 4x4xN SVE SIMD clone However, in the last one it may assume a VL for the codegen of the body and it's the user's responsibility to only use it for targets with that length , much like any other code produced this way. So that's what we tell the compiler is available and what the compiler generates depending on where we use the attribute. The question at hand here is, what can the vectorizer use for a specific loop. If we are using Advanced SIMD modes then it needs to call an Advanced SIMD clone, and if we are using SVE modes then it needs to call an SVE clone. At least until we support the ABI conversion, because like I said for an unpacked argument they behave differently. PS: In the future OpenMP may add specifications that allow us to define a specific VLA simdlen... in other words, whether we want [128, 128] or [256, 256], [512, 512] ... etc, but that still needs agreement on the OpenMP Spec, which is why for now we piggy back on the simdlen-less definition to provide us a VLA SVE simdclone with [128, 128] VL. Hopefully this makes things a bit clearer :/ > > That said, I wonder how we end up mixing things up in the first place. > > Richard.
On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > On 31/01/2024 14:35, Richard Biener wrote: > > On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > >> > >> > >> On 31/01/2024 13:58, Richard Biener wrote: > >>> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > >>> > >>>> > >>>> > >>>> On 31/01/2024 12:13, Richard Biener wrote: > >>>>> On Wed, 31 Jan 2024, Richard Biener wrote: > >>>>> > >>>>>> On Tue, 30 Jan 2024, Andre Vieira wrote: > >>>>>> > >>>>>>> > >>>>>>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure > >>>>>>> the > >>>>>>> target can reject a simd_clone based on the vector mode it is using. > >>>>>>> This is needed because for VLS SVE vectorization the vectorizer > >>>>>>> accepts > >>>>>>> Advanced SIMD simd clones when vectorizing using SVE types because the > >>>>>>> simdlens > >>>>>>> might match. This will cause type errors later on. > >>>>>>> > >>>>>>> Other targets do not currently need to use this argument. > >>>>>> > >>>>>> Can you instead pass down the mode? > >>>>> > >>>>> Thinking about that again the cgraph_simd_clone info in the clone > >>>>> should have sufficient information to disambiguate. If it doesn't > >>>>> then we should amend it. > >>>>> > >>>>> Richard. > >>>> > >>>> Hi Richard, > >>>> > >>>> Thanks for the review, I don't think cgraph_simd_clone_info is the right > >>>> place > >>>> to pass down this information, since this is information about the caller > >>>> rather than the simdclone itself. What we are trying to achieve here is > >>>> making > >>>> the vectorizer being able to accept or reject simdclones based on the ISA > >>>> we > >>>> are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we > >>>> use > >>>> modes, I am also not sure that's ideal but it is what we currently use. > >>>> So > >>>> to > >>>> answer your earlier question, yes I can also pass down mode if that's > >>>> preferable. > >>> > >>> Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere > >>> whether that's POLY or constant. I wonder how aarch64_sve_mode_p > >>> comes into play here which in the end classifies VLS SVE modes as > >>> non-SVE? > >>> > >> > >> Using -msve-vector-bits=128 > >> (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) > >> $4 = E_VNx4SImode > >> (gdb) p TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo)) > >> $5 = (tree) 0xfffff741c1b0 > >> (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) > >> 128 > >> (gdb) p aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo))) > >> $5 = true > >> > >> and for reference without vls codegen: > >> (gdb) p TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)) > >> $1 = E_VNx4SImode > >> (gdb) p debug (TYPE_SIZE (STMT_VINFO_VECTYPE (stmt_vinfo))) > >> POLY_INT_CST [128, 128] > >> > >> Having said that I believe that the USABLE targethook implementation for > >> aarch64 should also block other uses, like an Advanced SIMD mode being used > >> as > >> input for a SVE VLS SIMDCLONE. The reason being that for instance 'half' > >> registers like VNx2SI are packed differently from V2SI. > >> > >> We could teach the vectorizer to support these of course, but that requires > >> more work and is not extremely useful just yet. I'll add the extra check > >> that > >> to the patch once we agree on how to pass down the information we need. > >> Happy > >> to use either mode, or stmt_vec_info and extract the mode from it like it > >> does > >> now. > > > > As said, please pass down 'mode'. But I wonder how to document it, > > which mode is that supposed to be? Any of result or any argument > > mode that happens to be a vector? I think that we might be able > > to mix Advanced SIMD modes and SVE modes with -msve-vector-bits=128 > > in the same loop? > > > > Are the simd clones you don't want to use with -msve-vector-bits=128 > > having constant simdlen? If so why do you generate them in the first > > place? > > So this is where things get a bit confusing and I will write up some text for > these cases to put in our ABI document (currently in Beta and in need of some > tlc). > > Our intended behaviour is for a 'declare simd' without a simdlen to generate > simdclones for: > * Advanced SIMD 128 and 64-bit vectors, where possible (we don't allow for > simdlen 1, Tamar fixed that in gcc recently), > * SVE VLA vectors. > > Let me illustrate this with an example: > > __attribute__ ((simd (notinbranch), const)) float cosf(float); > > Should tell the compiler the following simd clones are available: > __ZGVnN4v_cosf 128-bit 4x4 float Advanced SIMD clone > __ZGVnN2v_cosf 64-bit 4x2 float Advanced SIMD clone > __ZGVsMxv_cosf [128, 128]-bit 4x4xN SVE SIMD clone > > [To save you looking into the abi let me break this down, _ZGV is prefix, then > 'n' or 's' picks between Advanced SIMD and SVE, 'N' or 'M' picks between Not > Masked and Masked (SVE is always masked even if we ask for notinbranch), then > a digit or 'x' picks between Vector Length or VLA, and after that you get a > letter per argument, where v = vector mapped] > > Regardless of -msve-vector-bits, however, the vectorizer (and any other part > of the compiler) may assume that the VL of the VLA SVE clone is that specified > by -msve-vector-bits, which if the clone is written in a VLA way will still > work. > > If the attribute is used with a function definition rather than declaration, > so: > > __attribute__ ((simd (notinbranch), const)) float fn0(float a) > { > return a + 1.0f; > } > > the compiler should again generate the three simd clones: > __ZGVnN4v_fn0 128-bit 4x4 float Advanced SIMD clone > __ZGVnN2v_fn0 64-bit 4x2 float Advanced SIMD clone > __ZGVsMxv_fn0 [128, 128]-bit 4x4xN SVE SIMD clone > > However, in the last one it may assume a VL for the codegen of the body and > it's the user's responsibility to only use it for targets with that length , > much like any other code produced this way. > > So that's what we tell the compiler is available and what the compiler > generates depending on where we use the attribute. The question at hand here > is, what can the vectorizer use for a specific loop. If we are using Advanced > SIMD modes then it needs to call an Advanced SIMD clone, and if we are using > SVE modes then it needs to call an SVE clone. At least until we support the > ABI conversion, because like I said for an unpacked argument they behave > differently. > > PS: In the future OpenMP may add specifications that allow us to define a > specific VLA simdlen... in other words, whether we want [128, 128] or [256, > 256], [512, 512] ... etc, but that still needs agreement on the OpenMP Spec, > which is why for now we piggy back on the simdlen-less definition to provide > us a VLA SVE simdclone with [128, 128] VL. > > Hopefully this makes things a bit clearer :/ So where does it go wrong? What case does the patch fix? For the non-definition case the SVE clone should have a POLY_INT simdlen and as you say it should be fine to use that even with -msve-vector-bits. For the definition case the SVE clone might have a constant simdlen but so does the caller (unless we allow different setting between functions/TUs?). The only thing the vectorizer looks at is I think if (!constant_multiple_p (vf * group_size, n->simdclone->simdlen, &num_calls) || (!n->simdclone->inbranch && (masked_call_offset > 0)) || (nargs != simd_nargs)) continue; plus your 2nd patch rejecting num_calls > 1 for variable-length SVE. The patch didn't come with a testcase so it's really hard to tell what goes wrong now and how it is fixed ... Richard. > > > > That said, I wonder how we end up mixing things up in the first place. > > > > Richard. >
"Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com> writes: > [...] The question at hand > here is, what can the vectorizer use for a specific loop. If we are > using Advanced SIMD modes then it needs to call an Advanced SIMD clone, > and if we are using SVE modes then it needs to call an SVE clone. At > least until we support the ABI conversion, because like I said for an > unpacked argument they behave differently. Probably also worth noting that multi-byte elements are laid out differently for big-endian. E.g. V4SI is loaded as a 128-bit integer whereas VNx4SI is loaded as an array of 4 32-bit integers, with the first 32-bit integer going in the least significant bits of the register. So it would only be possible to use Advanced SIMD clones for SVE modes and vice versa for little-endian, or if the elements are all bytes, or if we add some reverses to the inputs and outputs. Richard
On 01/02/2024 07:19, Richard Biener wrote: > On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > The patch didn't come with a testcase so it's really hard to tell > what goes wrong now and how it is fixed ... My bad! I had a testcase locally but never added it... However... now I look at it and ran it past Richard S, the codegen isn't 'wrong', but it does have the potential to lead to some pretty slow codegen, especially for inbranch simdclones where it transforms the SVE predicate into an Advanced SIMD vector by inserting the elements one at a time... An example of which can be seen if you do: gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 -fopenmp-simd t.c -S with the following t.c: #pragma omp declare simd simdlen(4) inbranch int __attribute__ ((const)) fn5(int); void fn4 (int *a, int *b, int n) { for (int i = 0; i < n; ++i) b[i] = fn5(a[i]); } Now I do have to say, for our main usecase of libmvec we won't have any 'inbranch' Advanced SIMD clones, so we avoid that issue... But of course that doesn't mean user-code will. I'm gonna remove this patch and run another test regression to see if it catches anything weird, but if not then I guess we do have the option to not use this patch and aim to solve the costing or codegen issue in GCC-15. We don't currently do any simdclone costing and I don't have a clear suggestion for how given openmp has no mechanism that I know off to expose the speedup of a simdclone over it's scalar variant, so how would we 'compare' a simdclone call with extra overhead of argument preparation vs scalar, though at least we could prefer a call to a different simdclone with less argument preparation. Anyways I digress. Other tests, these require aarch64-autovec-preference=2 so that also has me worried less... gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 --param aarch64-autovec-preference=2 -fopenmp-simd t.c -S t.c: #pragma omp declare simd simdlen(2) notinbranch float __attribute__ ((const)) fn1(double); void fn0 (float *a, float *b, int n) { for (int i = 0; i < n; ++i) b[i] = fn1((double) a[i]); } #pragma omp declare simd simdlen(2) notinbranch float __attribute__ ((const)) fn3(float); void fn2 (float *a, double *b, int n) { for (int i = 0; i < n; ++i) b[i] = (double) fn3(a[i]); } > Richard. > >>> >>> That said, I wonder how we end up mixing things up in the first place. >>> >>> Richard. >> >
On Thu, 1 Feb 2024, Andre Vieira (lists) wrote: > > > On 01/02/2024 07:19, Richard Biener wrote: > > On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > > > > > > The patch didn't come with a testcase so it's really hard to tell > > what goes wrong now and how it is fixed ... > > My bad! I had a testcase locally but never added it... > > However... now I look at it and ran it past Richard S, the codegen isn't > 'wrong', but it does have the potential to lead to some pretty slow codegen, > especially for inbranch simdclones where it transforms the SVE predicate into > an Advanced SIMD vector by inserting the elements one at a time... > > An example of which can be seen if you do: > > gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 -fopenmp-simd t.c -S > > with the following t.c: > #pragma omp declare simd simdlen(4) inbranch > int __attribute__ ((const)) fn5(int); > > void fn4 (int *a, int *b, int n) > { > for (int i = 0; i < n; ++i) > b[i] = fn5(a[i]); > } > > Now I do have to say, for our main usecase of libmvec we won't have any > 'inbranch' Advanced SIMD clones, so we avoid that issue... But of course that > doesn't mean user-code will. It seems to use SVE masks with vector(4) <signed-boolean:4> and the ABI says the mask is vector(4) int. You say that's because we choose a Adv SIMD clone for the SVE VLS vector code (it calls _ZGVnM4v_fn5). The vectorizer creates _44 = VEC_COND_EXPR <loop_mask_41, { 1, 1, 1, 1 }, { 0, 0, 0, 0 }>; and then vector lowering decomposes this. That means the vectorizer lacks a check that the target handles this VEC_COND_EXPR. Of course I would expect that SVE with VLS vectors is able to code generate this operation, so it's missing patterns in the end. Richard. > I'm gonna remove this patch and run another test regression to see if it > catches anything weird, but if not then I guess we do have the option to not > use this patch and aim to solve the costing or codegen issue in GCC-15. We > don't currently do any simdclone costing and I don't have a clear suggestion > for how given openmp has no mechanism that I know off to expose the speedup of > a simdclone over it's scalar variant, so how would we 'compare' a simdclone > call with extra overhead of argument preparation vs scalar, though at least we > could prefer a call to a different simdclone with less argument preparation. > Anyways I digress. > > Other tests, these require aarch64-autovec-preference=2 so that also has me > worried less... > > gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 --param > aarch64-autovec-preference=2 -fopenmp-simd t.c -S > > t.c: > #pragma omp declare simd simdlen(2) notinbranch > float __attribute__ ((const)) fn1(double); > > void fn0 (float *a, float *b, int n) > { > for (int i = 0; i < n; ++i) > b[i] = fn1((double) a[i]); > } > > #pragma omp declare simd simdlen(2) notinbranch > float __attribute__ ((const)) fn3(float); > > void fn2 (float *a, double *b, int n) > { > for (int i = 0; i < n; ++i) > b[i] = (double) fn3(a[i]); > } > > > Richard. > > > >>> > >>> That said, I wonder how we end up mixing things up in the first place. > >>> > >>> Richard. > >> > > > >
On 05/02/2024 09:56, Richard Biener wrote: > On Thu, 1 Feb 2024, Andre Vieira (lists) wrote: > >> >> >> On 01/02/2024 07:19, Richard Biener wrote: >>> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: >>> >>> >>> The patch didn't come with a testcase so it's really hard to tell >>> what goes wrong now and how it is fixed ... >> >> My bad! I had a testcase locally but never added it... >> >> However... now I look at it and ran it past Richard S, the codegen isn't >> 'wrong', but it does have the potential to lead to some pretty slow codegen, >> especially for inbranch simdclones where it transforms the SVE predicate into >> an Advanced SIMD vector by inserting the elements one at a time... >> >> An example of which can be seen if you do: >> >> gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 -fopenmp-simd t.c -S >> >> with the following t.c: >> #pragma omp declare simd simdlen(4) inbranch >> int __attribute__ ((const)) fn5(int); >> >> void fn4 (int *a, int *b, int n) >> { >> for (int i = 0; i < n; ++i) >> b[i] = fn5(a[i]); >> } >> >> Now I do have to say, for our main usecase of libmvec we won't have any >> 'inbranch' Advanced SIMD clones, so we avoid that issue... But of course that >> doesn't mean user-code will. > > It seems to use SVE masks with vector(4) <signed-boolean:4> and the > ABI says the mask is vector(4) int. You say that's because we choose > a Adv SIMD clone for the SVE VLS vector code (it calls _ZGVnM4v_fn5). > > The vectorizer creates > > _44 = VEC_COND_EXPR <loop_mask_41, { 1, 1, 1, 1 }, { 0, 0, 0, 0 }>; > > and then vector lowering decomposes this. That means the vectorizer > lacks a check that the target handles this VEC_COND_EXPR. > > Of course I would expect that SVE with VLS vectors is able to > code generate this operation, so it's missing patterns in the end. > > Richard. > What should we do for GCC-14? Going forward I think the right thing to do is to add these patterns. But I am not even going to try to do that right now and even though we can codegen for this, the result doesn't feel like it would ever be profitable which means I'd rather not vectorize, or well pick a different vector mode if possible. This would be achieved with the change to the targethook. If I change the hook to take modes, using STMT_VINFO_VECTYPE (stmt_vinfo), is that OK for now? Kind regards, Andre
On Mon, 26 Feb 2024, Andre Vieira (lists) wrote: > > > On 05/02/2024 09:56, Richard Biener wrote: > > On Thu, 1 Feb 2024, Andre Vieira (lists) wrote: > > > >> > >> > >> On 01/02/2024 07:19, Richard Biener wrote: > >>> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > >>> > >>> > >>> The patch didn't come with a testcase so it's really hard to tell > >>> what goes wrong now and how it is fixed ... > >> > >> My bad! I had a testcase locally but never added it... > >> > >> However... now I look at it and ran it past Richard S, the codegen isn't > >> 'wrong', but it does have the potential to lead to some pretty slow > >> codegen, > >> especially for inbranch simdclones where it transforms the SVE predicate > >> into > >> an Advanced SIMD vector by inserting the elements one at a time... > >> > >> An example of which can be seen if you do: > >> > >> gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 -fopenmp-simd t.c -S > >> > >> with the following t.c: > >> #pragma omp declare simd simdlen(4) inbranch > >> int __attribute__ ((const)) fn5(int); > >> > >> void fn4 (int *a, int *b, int n) > >> { > >> for (int i = 0; i < n; ++i) > >> b[i] = fn5(a[i]); > >> } > >> > >> Now I do have to say, for our main usecase of libmvec we won't have any > >> 'inbranch' Advanced SIMD clones, so we avoid that issue... But of course > >> that > >> doesn't mean user-code will. > > > > It seems to use SVE masks with vector(4) <signed-boolean:4> and the > > ABI says the mask is vector(4) int. You say that's because we choose > > a Adv SIMD clone for the SVE VLS vector code (it calls _ZGVnM4v_fn5). > > > > The vectorizer creates > > > > _44 = VEC_COND_EXPR <loop_mask_41, { 1, 1, 1, 1 }, { 0, 0, 0, 0 }>; > > > > and then vector lowering decomposes this. That means the vectorizer > > lacks a check that the target handles this VEC_COND_EXPR. > > > > Of course I would expect that SVE with VLS vectors is able to > > code generate this operation, so it's missing patterns in the end. > > > > Richard. > > > > What should we do for GCC-14? Going forward I think the right thing to do is > to add these patterns. But I am not even going to try to do that right now and > even though we can codegen for this, the result doesn't feel like it would > ever be profitable which means I'd rather not vectorize, or well pick a > different vector mode if possible. > > This would be achieved with the change to the targethook. If I change the hook > to take modes, using STMT_VINFO_VECTYPE (stmt_vinfo), is that OK for now? Passing in a mode is OK. I'm still not fully understanding why the clone isn't fully specifying 'mode' and if it does not why the vectorizer itself can not disregard it. From the past discussion I understood the existing situation isn't as bad as initially thought and no bad things happen right now? Thanks, Richard.
On 27/02/2024 08:47, Richard Biener wrote: > On Mon, 26 Feb 2024, Andre Vieira (lists) wrote: > >> >> >> On 05/02/2024 09:56, Richard Biener wrote: >>> On Thu, 1 Feb 2024, Andre Vieira (lists) wrote: >>> >>>> >>>> >>>> On 01/02/2024 07:19, Richard Biener wrote: >>>>> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: >>>>> >>>>> >>>>> The patch didn't come with a testcase so it's really hard to tell >>>>> what goes wrong now and how it is fixed ... >>>> >>>> My bad! I had a testcase locally but never added it... >>>> >>>> However... now I look at it and ran it past Richard S, the codegen isn't >>>> 'wrong', but it does have the potential to lead to some pretty slow >>>> codegen, >>>> especially for inbranch simdclones where it transforms the SVE predicate >>>> into >>>> an Advanced SIMD vector by inserting the elements one at a time... >>>> >>>> An example of which can be seen if you do: >>>> >>>> gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 -fopenmp-simd t.c -S >>>> >>>> with the following t.c: >>>> #pragma omp declare simd simdlen(4) inbranch >>>> int __attribute__ ((const)) fn5(int); >>>> >>>> void fn4 (int *a, int *b, int n) >>>> { >>>> for (int i = 0; i < n; ++i) >>>> b[i] = fn5(a[i]); >>>> } >>>> >>>> Now I do have to say, for our main usecase of libmvec we won't have any >>>> 'inbranch' Advanced SIMD clones, so we avoid that issue... But of course >>>> that >>>> doesn't mean user-code will. >>> >>> It seems to use SVE masks with vector(4) <signed-boolean:4> and the >>> ABI says the mask is vector(4) int. You say that's because we choose >>> a Adv SIMD clone for the SVE VLS vector code (it calls _ZGVnM4v_fn5). >>> >>> The vectorizer creates >>> >>> _44 = VEC_COND_EXPR <loop_mask_41, { 1, 1, 1, 1 }, { 0, 0, 0, 0 }>; >>> >>> and then vector lowering decomposes this. That means the vectorizer >>> lacks a check that the target handles this VEC_COND_EXPR. >>> >>> Of course I would expect that SVE with VLS vectors is able to >>> code generate this operation, so it's missing patterns in the end. >>> >>> Richard. >>> >> >> What should we do for GCC-14? Going forward I think the right thing to do is >> to add these patterns. But I am not even going to try to do that right now and >> even though we can codegen for this, the result doesn't feel like it would >> ever be profitable which means I'd rather not vectorize, or well pick a >> different vector mode if possible. >> >> This would be achieved with the change to the targethook. If I change the hook >> to take modes, using STMT_VINFO_VECTYPE (stmt_vinfo), is that OK for now? > > Passing in a mode is OK. I'm still not fully understanding why the > clone isn't fully specifying 'mode' and if it does not why the > vectorizer itself can not disregard it. We could check that the modes of the parameters & return type are the same as the vector operands & result in the vectorizer. But then we'd also want to make sure we don't reject cases where we have simdclones with compatible modes, aka same element type, but a multiple element count. Which is where'd we get in trouble again I think, because we'd want to accept V8SI -> 2x V4SI, but not V8SI -> 2x VNx4SI (with VLS and aarch64_sve_vg = 2), not because it's invalid, but because right now the codegen is bad. And it's easier to do this in the targethook, which we can technically also use to 'rank' simdclones by setting a target_badness value, so in the future we could decide to assign some 'badness' to influence the rank a SVE simdclone for Advanced SIMD loops vs an Advanced SIMD clone for Advanced SIMD loops. This does touch another issue of simdclone costing, which is a larger issue in general and one we (arm) might want to approach in the future. It's a complex issue, because the vectorizer doesn't know the performance impact of a simdclone, we assume (as we should) that its faster than the original scalar, though we currently don't record costs for either, but we don't know by how much or how much impact it has, so the vectorizer can't reason whether it's beneficial to use a simdclone if it has to do a lot of operand preparation, we can merely tell it to use it, or not and all the other operations in the loop will determine costing. > From the past discussion I understood the existing situation isn't > as bad as initially thought and no bad things happen right now? Nope, I thought they compiler would fall apart, but it seems to be able to transform the operands from one mode into the other, so without the targethook it just generates slower loops in certain cases, which we'd rather avoid given the usecase for simdclones is to speed things up ;) Attached reworked patch. This patch adds a machine_mode argument to TARGET_SIMD_CLONE_USABLE to make sure the target can reject a simd_clone based on the vector mode it is using. This is needed because for VLS SVE vectorization the vectorizer accepts Advanced SIMD simd clones when vectorizing using SVE types because the simdlens might match, this currently leads to suboptimal codegen. Other targets do not currently need to use this argument. gcc/ChangeLog: * target.def (TARGET_SIMD_CLONE_USABLE): Add argument. * tree-vect-stmts.cc (vectorizable_simd_clone_call): Pass vector_mode to call TARGET_SIMD_CLONE_USABLE. * config/aarch64/aarch64.cc (aarch64_simd_clone_usable): Add argument and use it to reject the use of SVE simd clones with Advanced SIMD modes. * config/gcn/gcn.cc (gcn_simd_clone_usable): Add unused argument. * config/i386/i386.cc (ix86_simd_clone_usable): Likewise. diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index 16318bf925883ecedf9345e53fc0824a553b2747..6ee77f61235219b477d1f622fceb752d54c58b87 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -28769,12 +28769,12 @@ aarch64_simd_clone_adjust (struct cgraph_node *node) /* Implement TARGET_SIMD_CLONE_USABLE. */ static int -aarch64_simd_clone_usable (struct cgraph_node *node) +aarch64_simd_clone_usable (struct cgraph_node *node, machine_mode vector_mode) { switch (node->simdclone->vecsize_mangle) { case 'n': - if (!TARGET_SIMD) + if (!TARGET_SIMD || aarch64_sve_mode_p (vector_mode)) return -1; return 0; default: diff --git a/gcc/config/gcn/gcn.cc b/gcc/config/gcn/gcn.cc index bc076d1120d9e7d03c9bed23b8df215ae35e442c..9624b7c1aab29665c52f7b82d8b437af2e8e1ea1 100644 --- a/gcc/config/gcn/gcn.cc +++ b/gcc/config/gcn/gcn.cc @@ -5667,7 +5667,8 @@ gcn_simd_clone_adjust (struct cgraph_node *ARG_UNUSED (node)) /* Implement TARGET_SIMD_CLONE_USABLE. */ static int -gcn_simd_clone_usable (struct cgraph_node *ARG_UNUSED (node)) +gcn_simd_clone_usable (struct cgraph_node *ARG_UNUSED (node), + machine_mode ARG_UNUSED (vector_mode)) { /* We don't need to do anything here because gcn_simd_clone_compute_vecsize_and_simdlen currently only returns one diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index fc5068539c11748a5adf70ec77b2f1cae1a1e231..c54f66543fdd4103d58c2f9390a3c91060597b94 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -25249,7 +25249,8 @@ ix86_simd_clone_compute_vecsize_and_simdlen (struct cgraph_node *node, slightly less desirable, etc.). */ static int -ix86_simd_clone_usable (struct cgraph_node *node) +ix86_simd_clone_usable (struct cgraph_node *node, + machine_mode ARG_UNUSED (vector_mode)) { switch (node->simdclone->vecsize_mangle) { diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index c8b8b126b2424b6552f824ba42ac329cfaf84d84..03f7d72a429204a584253dc5c6e8fa1b3074795d 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -6498,11 +6498,11 @@ This hook should add implicit @code{attribute(target("..."))} attribute to SIMD clone @var{node} if needed. @end deftypefn -@deftypefn {Target Hook} int TARGET_SIMD_CLONE_USABLE (struct cgraph_node *@var{}) +@deftypefn {Target Hook} int TARGET_SIMD_CLONE_USABLE (struct cgraph_node *@var{}, @var{machine_mode}) This hook should return -1 if SIMD clone @var{node} shouldn't be used -in vectorized loops in current function, or non-negative number if it is -usable. In that case, the smaller the number is, the more desirable it is -to use it. +in vectorized loops in current function with @var{vector_mode}, or +non-negative number if it is usable. In that case, the smaller the number +is, the more desirable it is to use it. @end deftypefn @deftypefn {Target Hook} int TARGET_SIMT_VF (void) diff --git a/gcc/target.def b/gcc/target.def index fdad7bbc93e2ad8aea30336d5cd4af67801e9c74..7e8921b6bd4078770268819a38595fdce612b548 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -1645,10 +1645,10 @@ void, (struct cgraph_node *), NULL) DEFHOOK (usable, "This hook should return -1 if SIMD clone @var{node} shouldn't be used\n\ -in vectorized loops in current function, or non-negative number if it is\n\ -usable. In that case, the smaller the number is, the more desirable it is\n\ -to use it.", -int, (struct cgraph_node *), NULL) +in vectorized loops in current function with @var{vector_mode}, or\n\ +non-negative number if it is usable. In that case, the smaller the number\n\ +is, the more desirable it is to use it.", +int, (struct cgraph_node *, machine_mode), NULL) HOOK_VECTOR_END (simd_clone) diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc index 1dbe1115da4d7dd4fc590e5830a9c7f05be6945a..f06a53d37ee05737e00e80d9c265192bede6aa18 100644 --- a/gcc/tree-vect-stmts.cc +++ b/gcc/tree-vect-stmts.cc @@ -4074,7 +4074,14 @@ vectorizable_simd_clone_call (vec_info *vinfo, stmt_vec_info stmt_info, this_badness += floor_log2 (num_calls) * 4096; if (n->simdclone->inbranch) this_badness += 8192; - int target_badness = targetm.simd_clone.usable (n); + + /* If STMT_VINFO_VECTYPE has not been set yet pass the general vector + mode, which for targets that use it will determine what ISA we can + vectorize this code with. */ + machine_mode vector_mode = vinfo->vector_mode; + if (vectype) + vector_mode = TYPE_MODE (vectype); + int target_badness = targetm.simd_clone.usable (n, vector_mode); if (target_badness < 0) continue; this_badness += target_badness * 512;
On Wed, 28 Feb 2024, Andre Vieira (lists) wrote: > > > On 27/02/2024 08:47, Richard Biener wrote: > > On Mon, 26 Feb 2024, Andre Vieira (lists) wrote: > > > >> > >> > >> On 05/02/2024 09:56, Richard Biener wrote: > >>> On Thu, 1 Feb 2024, Andre Vieira (lists) wrote: > >>> > >>>> > >>>> > >>>> On 01/02/2024 07:19, Richard Biener wrote: > >>>>> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote: > >>>>> > >>>>> > >>>>> The patch didn't come with a testcase so it's really hard to tell > >>>>> what goes wrong now and how it is fixed ... > >>>> > >>>> My bad! I had a testcase locally but never added it... > >>>> > >>>> However... now I look at it and ran it past Richard S, the codegen isn't > >>>> 'wrong', but it does have the potential to lead to some pretty slow > >>>> codegen, > >>>> especially for inbranch simdclones where it transforms the SVE predicate > >>>> into > >>>> an Advanced SIMD vector by inserting the elements one at a time... > >>>> > >>>> An example of which can be seen if you do: > >>>> > >>>> gcc -O3 -march=armv8-a+sve -msve-vector-bits=128 -fopenmp-simd t.c -S > >>>> > >>>> with the following t.c: > >>>> #pragma omp declare simd simdlen(4) inbranch > >>>> int __attribute__ ((const)) fn5(int); > >>>> > >>>> void fn4 (int *a, int *b, int n) > >>>> { > >>>> for (int i = 0; i < n; ++i) > >>>> b[i] = fn5(a[i]); > >>>> } > >>>> > >>>> Now I do have to say, for our main usecase of libmvec we won't have any > >>>> 'inbranch' Advanced SIMD clones, so we avoid that issue... But of course > >>>> that > >>>> doesn't mean user-code will. > >>> > >>> It seems to use SVE masks with vector(4) <signed-boolean:4> and the > >>> ABI says the mask is vector(4) int. You say that's because we choose > >>> a Adv SIMD clone for the SVE VLS vector code (it calls _ZGVnM4v_fn5). > >>> > >>> The vectorizer creates > >>> > >>> _44 = VEC_COND_EXPR <loop_mask_41, { 1, 1, 1, 1 }, { 0, 0, 0, 0 }>; > >>> > >>> and then vector lowering decomposes this. That means the vectorizer > >>> lacks a check that the target handles this VEC_COND_EXPR. > >>> > >>> Of course I would expect that SVE with VLS vectors is able to > >>> code generate this operation, so it's missing patterns in the end. > >>> > >>> Richard. > >>> > >> > >> What should we do for GCC-14? Going forward I think the right thing to do > >> is > >> to add these patterns. But I am not even going to try to do that right now > >> and > >> even though we can codegen for this, the result doesn't feel like it would > >> ever be profitable which means I'd rather not vectorize, or well pick a > >> different vector mode if possible. > >> > >> This would be achieved with the change to the targethook. If I change the > >> hook > >> to take modes, using STMT_VINFO_VECTYPE (stmt_vinfo), is that OK for now? > > > > Passing in a mode is OK. I'm still not fully understanding why the > > clone isn't fully specifying 'mode' and if it does not why the > > vectorizer itself can not disregard it. > > > We could check that the modes of the parameters & return type are the same as > the vector operands & result in the vectorizer. But then we'd also want to > make sure we don't reject cases where we have simdclones with compatible > modes, aka same element type, but a multiple element count. Which is where'd > we get in trouble again I think, because we'd want to accept V8SI -> 2x V4SI, > but not V8SI -> 2x VNx4SI (with VLS and aarch64_sve_vg = 2), not because it's > invalid, but because right now the codegen is bad. And it's easier to do this > in the targethook, which we can technically also use to 'rank' simdclones by > setting a target_badness value, so in the future we could decide to assign > some 'badness' to influence the rank a SVE simdclone for Advanced SIMD loops > vs an Advanced SIMD clone for Advanced SIMD loops. > > This does touch another issue of simdclone costing, which is a larger issue in > general and one we (arm) might want to approach in the future. It's a complex > issue, because the vectorizer doesn't know the performance impact of a > simdclone, we assume (as we should) that its faster than the original scalar, > though we currently don't record costs for either, but we don't know by how > much or how much impact it has, so the vectorizer can't reason whether it's > beneficial to use a simdclone if it has to do a lot of operand preparation, we > can merely tell it to use it, or not and all the other operations in the loop > will determine costing. > > > > From the past discussion I understood the existing situation isn't > > as bad as initially thought and no bad things happen right now? > Nope, I thought they compiler would fall apart, but it seems to be able to > transform the operands from one mode into the other, so without the targethook > it just generates slower loops in certain cases, which we'd rather avoid given > the usecase for simdclones is to speed things up ;) > > > Attached reworked patch. > > > This patch adds a machine_mode argument to TARGET_SIMD_CLONE_USABLE to make > sure the target can reject a simd_clone based on the vector mode it is using. > This is needed because for VLS SVE vectorization the vectorizer accepts > Advanced SIMD simd clones when vectorizing using SVE types because the > simdlens might match, this currently leads to suboptimal codegen. > > Other targets do not currently need to use this argument. +ix86_simd_clone_usable (struct cgraph_node *node, + machine_mode ARG_UNUSED (vector_mode)) we use C++, just omit the parameter name. You use STMT_VINFO_VECTYPE conditional and vinfo->vector_mode otherwise. I think simdclones without a return value might be a thing? What type would STMT_VINFO_VECTYPE correspond to? The documentation also doesn't say whether it's the mode of a return value, or which argument value. It seems it is just a random mode that might or might not provide properties of the incoming(?) argument values and that might differ from the actual argument modes? As said, I think the vectorizer has more info - it knows the incoming mode for each arg and the expected argument modes for each arg, including mask modes involved (that extra mode argument is for values and never for masks?). Anyway, I can live with this but I'll leave it to Richard S. to approve (and take the blame ;)). Thanks, Richard. > gcc/ChangeLog: > > * target.def (TARGET_SIMD_CLONE_USABLE): Add argument. > * tree-vect-stmts.cc (vectorizable_simd_clone_call): Pass vector_mode > to call TARGET_SIMD_CLONE_USABLE. > * config/aarch64/aarch64.cc (aarch64_simd_clone_usable): Add argument > and use it to reject the use of SVE simd clones with Advanced SIMD > modes. > * config/gcn/gcn.cc (gcn_simd_clone_usable): Add unused argument. > * config/i386/i386.cc (ix86_simd_clone_usable): Likewise. >
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc index a37d47b243e..31617510160 100644 --- a/gcc/config/aarch64/aarch64.cc +++ b/gcc/config/aarch64/aarch64.cc @@ -28694,13 +28694,16 @@ aarch64_simd_clone_adjust (struct cgraph_node *node) /* Implement TARGET_SIMD_CLONE_USABLE. */ static int -aarch64_simd_clone_usable (struct cgraph_node *node) +aarch64_simd_clone_usable (struct cgraph_node *node, stmt_vec_info stmt_vinfo) { switch (node->simdclone->vecsize_mangle) { case 'n': if (!TARGET_SIMD) return -1; + if (STMT_VINFO_VECTYPE (stmt_vinfo) + && aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE (stmt_vinfo)))) + return -1; return 0; default: gcc_unreachable (); diff --git a/gcc/config/gcn/gcn.cc b/gcc/config/gcn/gcn.cc index e80de2ce056..c48b212d9e6 100644 --- a/gcc/config/gcn/gcn.cc +++ b/gcc/config/gcn/gcn.cc @@ -5658,7 +5658,8 @@ gcn_simd_clone_adjust (struct cgraph_node *ARG_UNUSED (node)) /* Implement TARGET_SIMD_CLONE_USABLE. */ static int -gcn_simd_clone_usable (struct cgraph_node *ARG_UNUSED (node)) +gcn_simd_clone_usable (struct cgraph_node *ARG_UNUSED (node), + stmt_vec_info ARG_UNUSED (stmt_vinfo)) { /* We don't need to do anything here because gcn_simd_clone_compute_vecsize_and_simdlen currently only returns one diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index b3e7c74846e..63e6b9d2643 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -25193,7 +25193,8 @@ ix86_simd_clone_compute_vecsize_and_simdlen (struct cgraph_node *node, slightly less desirable, etc.). */ static int -ix86_simd_clone_usable (struct cgraph_node *node) +ix86_simd_clone_usable (struct cgraph_node *node, + stmt_vec_info ARG_UNUSED (stmt_vinfo)) { switch (node->simdclone->vecsize_mangle) { diff --git a/gcc/target.def b/gcc/target.def index fdad7bbc93e..4fade9c4eec 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -1648,7 +1648,7 @@ DEFHOOK in vectorized loops in current function, or non-negative number if it is\n\ usable. In that case, the smaller the number is, the more desirable it is\n\ to use it.", -int, (struct cgraph_node *), NULL) +int, (struct cgraph_node *, _stmt_vec_info *), NULL) HOOK_VECTOR_END (simd_clone) diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc index 1dbe1115da4..da02082c034 100644 --- a/gcc/tree-vect-stmts.cc +++ b/gcc/tree-vect-stmts.cc @@ -4074,7 +4074,7 @@ vectorizable_simd_clone_call (vec_info *vinfo, stmt_vec_info stmt_info, this_badness += floor_log2 (num_calls) * 4096; if (n->simdclone->inbranch) this_badness += 8192; - int target_badness = targetm.simd_clone.usable (n); + int target_badness = targetm.simd_clone.usable (n, stmt_info); if (target_badness < 0) continue; this_badness += target_badness * 512;