| Message ID | 52a9b704f61e4780cd78d86c8ef25d356b378450.camel@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000] Add command line and builtin compatibility | expand |
On 3/11/20 2:00 PM, Carl Love wrote: > GCC maintianers: > > The following patch add a check to make sure the user did not specify > -mno_fprnd with the builtins __builtin_vsx_xsrdpim and > __builtin_vsx_xsrdpip. These builtins are incompatible with the > -mno_fprnd command line. The check prevents GCC crashing under these > conditions. > > Manually tested the patch on > > powerpc64le-unknown-linux-gnu (Power 8 LE) > powerpc64le-unknown-linux-gnu (Power 9 LE) > > as follows: > > gcc -mno-fprnd -g -c vsx-builtin-3.c > vsx-builtin-3.c: In function ‘do_math’: > vsx-builtin-3.c:145:3: error: __builtin_vsx_xsrdpim is incompatible > with mno-fprnd option > 145 | z[i][0] = __builtin_vsx_xsrdpim (z[i][1]); i++; > | ^ > vsx-builtin-3.c:146:3: error: __builtin_vsx_xsrdpip is incompatible > with mno-fprnd option > 146 | z[i][0] = __builtin_vsx_xsrdpip (z[i][1]); i++; > | ^ > > I read thru the source code looking for other builtins with the same > issue. I also created a script to compile all of the tests in > gcc/testsuite/gcc.target/powerpc with the -mno-fprnd option to check > for additional builtins that are incompatible with the -mno-fprnd > option. These were the only two builtins that were identified as being > incompatible with the -mno-fprnd option. > > Please let me know if the patch looks OK for mainline. Thanks. > > Carl Love > > ----------------------------------------------------------------------- > rs6000: Add command line and builtin compatibility check > > PR/target 87583 > > gcc/ChangeLog > > 2020-03-10 Carl Love <cel@us.ibm.com> > > * gcc/config/rs6000/rs6000-c.c > (altivec_resolve_overloaded_builtin): > Add check for TARGET_FRND and VSX_BUILTIN_XSRDPIM, > VSX_BUILTIN_XSRDPIP > compatibility. > --- > gcc/config/rs6000/rs6000-c.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000- > c.c > index 8c1fbbf..6820782 100644 > --- a/gcc/config/rs6000/rs6000-c.c > +++ b/gcc/config/rs6000/rs6000-c.c > @@ -915,6 +915,14 @@ altivec_resolve_overloaded_builtin (location_t > loc, tree fndecl, > const struct altivec_builtin_types *desc; > unsigned int n; > > + /* Check builtin for command line argument conflicts. */ > + if (!TARGET_FPRND && > + (fcode == VSX_BUILTIN_XSRDPIM || fcode == VSX_BUILTIN_XSRDPIP)) > { > + error ("%s is incompatible with mno-fprnd option", I believe you need %qs here. Also replace mno-fprnd with %qs and put "-mno-fprnd" as the associated parameter. Example from nearby code: error ("%qs requires %qs", "-mdirect-move", "-mvsx"); Thanks, Bill > + IDENTIFIER_POINTER (DECL_NAME (fndecl))); > + return error_mark_node; > + } > + > if (!rs6000_overloaded_builtin_p (fcode)) > return NULL_TREE; >
Bill: On Wed, 2020-03-11 at 14:12 -0500, Bill Schmidt wrote: > I believe you need %qs here. Also replace mno-fprnd with %qs and > put > "-mno-fprnd" as the associated parameter. > > Example from nearby code: error ("%qs requires %qs", "-mdirect- > move", > "-mvsx"); Yes. I had originally tried to put in -mno-fprnd in the message and got compilation errors. Had to drop the leading dash to get it to compile. The %qs fixes that! I re-did the patch, retested on Power 8 and Power 9. The test results are now: gcc -g -mno-fprnd vsx-builtin-3.c -o vsx-builtin-3 vsx-builtin-3.c: In function ‘do_math’: vsx-builtin-3.c:145:3: error: ‘__builtin_vsx_xsrdpim’ is incompatible with ‘-mno-fprnd’ option 145 | z[i][0] = __builtin_vsx_xsrdpim (z[i][1]); i++; | ^ vsx-builtin-3.c:146:3: error: ‘__builtin_vsx_xsrdpip’ is incompatible with ‘-mno-fprnd’ option 146 | z[i][0] = __builtin_vsx_xsrdpip (z[i][1]); i++; | ^ The updated patch is below. Please let me know if there are any additional things needing fixing for mainline. Thanks. Carl Love --------------------------------------------------------------------- rs6000: Add command line and builtin compatibility check PR/target 87583 gcc/ChangeLog 2020-03-11 Carl Love <cel@us.ibm.com> * gcc/config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin): Add check for TARGET_FRND and VSX_BUILTIN_XSRDPIM, VSX_BUILTIN_XSRDPIP compatibility. --- gcc/config/rs6000/rs6000-c.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c index 8c1fbbf..558e829 100644 --- a/gcc/config/rs6000/rs6000-c.c +++ b/gcc/config/rs6000/rs6000-c.c @@ -915,6 +915,14 @@ altivec_resolve_overloaded_builtin (location_t loc, tree fndecl, const struct altivec_builtin_types *desc; unsigned int n; + /* Check builtin for command line argument conflicts. */ + if (!TARGET_FPRND && + (fcode == VSX_BUILTIN_XSRDPIM || fcode == VSX_BUILTIN_XSRDPIP)) { + error ("%qs is incompatible with %qs option", + IDENTIFIER_POINTER (DECL_NAME (fndecl)), "-mno-fprnd"); + return error_mark_node; + } + if (!rs6000_overloaded_builtin_p (fcode)) return NULL_TREE;
Hi! On Wed, Mar 11, 2020 at 12:00:12PM -0700, Carl Love wrote: > The following patch add a check to make sure the user did not specify > -mno_fprnd with the builtins __builtin_vsx_xsrdpim and > __builtin_vsx_xsrdpip. These builtins are incompatible with the > -mno_fprnd command line. The check prevents GCC crashing under these > conditions. (-mno-fprnd, a dash, not an underscore) -mfprnd means all new insns in ISA 2.04; we should never allow this option together with a -mcpu= that implies 2.04 or higher. xsrdpi* require ISA 2.06 (Power7), so this testcase should work *anyway*, even if fri* would be disabled for some strange reason. > I read thru the source code looking for other builtins with the same > issue. From the GCC manual: -mmfcrf p4 2.01 -mpopcntb p5 2.02 -mfprnd p5+ 2.04 ("info gcc" says 2.03, that's wrong? But the ISA says this is 2.02 even? Now what!) -mcmpb p6 2.05 -mpopcntd p7 2.06 (and there are more, in fact). > 2020-03-10 Carl Love <cel@us.ibm.com> > > * gcc/config/rs6000/rs6000-c.c > (altivec_resolve_overloaded_builtin): > Add check for TARGET_FRND and VSX_BUILTIN_XSRDPIM, > VSX_BUILTIN_XSRDPIP > compatibility. FPRND > --- a/gcc/config/rs6000/rs6000-c.c > +++ b/gcc/config/rs6000/rs6000-c.c > @@ -915,6 +915,14 @@ altivec_resolve_overloaded_builtin (location_t > loc, tree fndecl, > const struct altivec_builtin_types *desc; > unsigned int n; > > + /* Check builtin for command line argument conflicts. */ > + if (!TARGET_FPRND && > + (fcode == VSX_BUILTIN_XSRDPIM || fcode == VSX_BUILTIN_XSRDPIP)) Lines never end in binary operators: that goes to the start of the next line, instead, so if (!TARGET_FPRND && (fcode == VSX_BUILTIN_XSRDPIM || fcode == VSX_BUILTIN_XSRDPIP)) I'd write that the other way around, it reads nicer: if ((fcode == VSX_BUILTIN_XSRDPIM || fcode == VSX_BUILTIN_XSRDPIP) && !TARGET_FPRND) but maybe that is just taste :-) > { > + error ("%s is incompatible with mno-fprnd option", > + IDENTIFIER_POINTER (DECL_NAME (fndecl))); > + return error_mark_node; > + } -mno-fprnd (options start with a dash, except in the first field in rs6000.opt). It would be better if you disallowed this option combination? Don't allow an options that says ISA X insns are allowed, but ISA Y insns are not, with Y < X. In this case X is 2.06 and Y is 2.02 or 2.03 or 2.04. Segher
On Wed, Mar 11, 2020 at 02:12:59PM -0500, Bill Schmidt wrote: > >+ error ("%s is incompatible with mno-fprnd option", > > I believe you need %qs here. Also replace mno-fprnd with %qs and put > "-mno-fprnd" as the associated parameter. > > Example from nearby code: error ("%qs requires %qs", "-mdirect-move", > "-mvsx"); Yes, that makes the translators' job easier (and forces more consistent output quoting, etc.) Segher
Segher: > > From the GCC manual: > > -mmfcrf p4 2.01 > -mpopcntb p5 2.02 > -mfprnd p5+ 2.04 ("info gcc" says 2.03, that's wrong? But the > ISA > says this is 2.02 even? Now what!) > -mcmpb p6 2.05 > -mpopcntd p7 2.06 > > (and there are more, in fact). <snip> > > It would be better if you disallowed this option combination? Don't > allow an options that says ISA X insns are allowed, but ISA Y insns > are > not, with Y < X. In this case X is 2.06 and Y is 2.02 or 2.03 or > 2.04. My fix was very specific to the builtin and the command line argument as pointed out in the bug report. Sounds like you are implying we should take a much more general approach to checking the command line args against the ISA level. Such a test would need to go somewhere else not in the builtin expansion call. We may want a routine that just does these checks which can then be called from the appropriate place. Let me go look at the GCC manual and see about what all needs testing. Carl
Hi Carl, On Thu, Mar 12, 2020 at 09:57:06AM -0700, Carl Love wrote: > > From the GCC manual: > > > > -mmfcrf p4 2.01 > > -mpopcntb p5 2.02 > > -mfprnd p5+ 2.04 ("info gcc" says 2.03, that's wrong? But the > > ISA > > says this is 2.02 even? Now what!) > > -mcmpb p6 2.05 > > -mpopcntd p7 2.06 > > > > (and there are more, in fact). > > <snip> > > > It would be better if you disallowed this option combination? Don't > > allow an options that says ISA X insns are allowed, but ISA Y insns > > are > > not, with Y < X. In this case X is 2.06 and Y is 2.02 or 2.03 or > > 2.04. > > My fix was very specific to the builtin and the command line argument > as pointed out in the bug report. > > Sounds like you are implying we should take a much more general > approach to checking the command line args against the ISA level. Such > a test would need to go somewhere else not in the builtin expansion > call. We may want a routine that just does these checks which can then > be called from the appropriate place. Let me go look at the GCC manual > and see about what all needs testing. Yes, but only for this fprnd vs. 2.06 (vsx) situation. Like we already have: if (TARGET_DIRECT_MOVE && !TARGET_VSX) { if (rs6000_isa_flags_explicit & OPTION_MASK_DIRECT_MOVE) error ("%qs requires %qs", "-mdirect-move", "-mvsx"); rs6000_isa_flags &= ~OPTION_MASK_DIRECT_MOVE; } (and many other cases there), we could do this there as well (so, don't allow -mvsx (maybe via a -mcpu= etc.) at the same time as -mno-fprnd). Do you see problems with that? Segher
Segher: > > Yes, but only for this fprnd vs. 2.06 (vsx) situation. Like we > already > have: > > if (TARGET_DIRECT_MOVE && !TARGET_VSX) > { > if (rs6000_isa_flags_explicit & OPTION_MASK_DIRECT_MOVE) > error ("%qs requires %qs", "-mdirect-move", "-mvsx"); > rs6000_isa_flags &= ~OPTION_MASK_DIRECT_MOVE; > } > > (and many other cases there), we could do this there as well (so, > don't > allow -mvsx (maybe via a -mcpu= etc.) at the same time as -mno- > fprnd). > I redid the patch to try and make it more general. It looks to me like TARGET_VSX is set for Power 7 and newer. I setup a test similar to the example checking TARGET_VSX. So if you are on a Power 7 then -mvsx is set for you, i.e. the user would not have to explicitly use the option. My objection to the error message in the example is that the user wouldn't necessarily know what processor or ISA is implied by -mvsx. So in my error message I called out the processor number. We could do it based on ISA. I figure the user is more likely to know the processor version then the ISA level supported by the processor so went with the processor number in the patch. Thoughts? gcc -mno-fprnd -g -mcpu=power7 -c vsx-builtin-3.c cc1: error: ‘-mno-fprnd’ not compatible with Power 7 and newer Carl Love --------------------------------------------------------------- From 212d2521437e7c32801b851bf9e23a9a12418de0 Mon Sep 17 00:00:00 2001 From: Carl Love <carll@us.ibm.com> Date: Wed, 11 Mar 2020 14:33:31 -0500 Subject: [PATCH] rs6000: Add command line and builtin compatibility check PR/target 87583 gcc/ChangeLog 2020-03-18 Carl Love <cel@us.ibm.com> * gcc/config/rs6000/rs6000.c (altivec_resolve_overloaded_builtin): Add check for TARGET_FRND for Power 7 or newer. --- gcc/config/rs6000/rs6000.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index ac9455e3b7c..5c72a863dbf 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -3716,6 +3716,14 @@ rs6000_option_override_internal (bool global_init_p) rs6000_isa_flags &= ~OPTION_MASK_CRYPTO; } + if (!TARGET_FPRND && TARGET_VSX) + { + if (rs6000_isa_flags_explicit & OPTION_MASK_FPRND) + /* TARGET_VSX = 1 implies Power 7 and newer */ + error ("%qs not compatible with Power 7 and newer", "-mno-fprnd"); + rs6000_isa_flags &= ~OPTION_MASK_FPRND; + } + if (TARGET_DIRECT_MOVE && !TARGET_VSX) { if (rs6000_isa_flags_explicit & OPTION_MASK_DIRECT_MOVE)
Hi Carl, On Wed, Mar 18, 2020 at 04:19:18PM -0700, Carl Love wrote: > > Yes, but only for this fprnd vs. 2.06 (vsx) situation. Like we > > already > > have: > > > > if (TARGET_DIRECT_MOVE && !TARGET_VSX) > > { > > if (rs6000_isa_flags_explicit & OPTION_MASK_DIRECT_MOVE) > > error ("%qs requires %qs", "-mdirect-move", "-mvsx"); > > rs6000_isa_flags &= ~OPTION_MASK_DIRECT_MOVE; > > } > > > > (and many other cases there), we could do this there as well (so, > > don't > > allow -mvsx (maybe via a -mcpu= etc.) at the same time as -mno- > > fprnd). > > I redid the patch to try and make it more general. It looks to me like > TARGET_VSX is set for Power 7 and newer. By default, yes. But you can use -mno-vsx, and you can use -mvsx with older CPUs as well (but you really really really shouldn't). > I setup a test similar to the > example checking TARGET_VSX. So if you are on a Power 7 then -mvsx is > set for you, i.e. the user would not have to explicitly use the option. > My objection to the error message in the example is that the user > wouldn't necessarily know what processor or ISA is implied by -mvsx. > So in my error message I called out the processor number. We could do > it based on ISA. I figure the user is more likely to know the > processor version then the ISA level supported by the processor so went > with the processor number in the patch. Thoughts? > > gcc -mno-fprnd -g -mcpu=power7 -c vsx-builtin-3.c > cc1: error: ‘-mno-fprnd’ not compatible with Power 7 and newer I think it should say error ("%qs requires %qs", "-mvsx", "-mfprnd"); (It's easier to not use negatives, and, this is more consistent with other such tests). > * gcc/config/rs6000/rs6000.c (altivec_resolve_overloaded_builtin): > Add check for TARGET_FRND for Power 7 or newer. (It's in a different function now, and, TARGET_FPRND). > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c > index ac9455e3b7c..5c72a863dbf 100644 > --- a/gcc/config/rs6000/rs6000.c > +++ b/gcc/config/rs6000/rs6000.c > @@ -3716,6 +3716,14 @@ rs6000_option_override_internal (bool global_init_p) > rs6000_isa_flags &= ~OPTION_MASK_CRYPTO; > } > > + if (!TARGET_FPRND && TARGET_VSX) > + { > + if (rs6000_isa_flags_explicit & OPTION_MASK_FPRND) > + /* TARGET_VSX = 1 implies Power 7 and newer */ > + error ("%qs not compatible with Power 7 and newer", "-mno-fprnd"); > + rs6000_isa_flags &= ~OPTION_MASK_FPRND; > + } Please make such changes if you agree. Either way, okay for trunk. Thank you, and sorry the review took so long. Segher
Hi!
On Thu, Mar 19, 2020 at 07:46:53PM -0500, Segher Boessenkool wrote:
> Please make such changes if you agree. Either way, okay for trunk.
Oh, and okay for backport to 9 next week :-)
Segher
diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000- c.c index 8c1fbbf..6820782 100644 --- a/gcc/config/rs6000/rs6000-c.c +++ b/gcc/config/rs6000/rs6000-c.c @@ -915,6 +915,14 @@ altivec_resolve_overloaded_builtin (location_t loc, tree fndecl, const struct altivec_builtin_types *desc; unsigned int n; + /* Check builtin for command line argument conflicts. */ + if (!TARGET_FPRND && + (fcode == VSX_BUILTIN_XSRDPIM || fcode == VSX_BUILTIN_XSRDPIP)) { + error ("%s is incompatible with mno-fprnd option", + IDENTIFIER_POINTER (DECL_NAME (fndecl))); + return error_mark_node; + } + if (!rs6000_overloaded_builtin_p (fcode))
GCC maintianers: The following patch add a check to make sure the user did not specify -mno_fprnd with the builtins __builtin_vsx_xsrdpim and __builtin_vsx_xsrdpip. These builtins are incompatible with the -mno_fprnd command line. The check prevents GCC crashing under these conditions. Manually tested the patch on powerpc64le-unknown-linux-gnu (Power 8 LE) powerpc64le-unknown-linux-gnu (Power 9 LE) as follows: gcc -mno-fprnd -g -c vsx-builtin-3.c vsx-builtin-3.c: In function ‘do_math’: vsx-builtin-3.c:145:3: error: __builtin_vsx_xsrdpim is incompatible with mno-fprnd option 145 | z[i][0] = __builtin_vsx_xsrdpim (z[i][1]); i++; | ^ vsx-builtin-3.c:146:3: error: __builtin_vsx_xsrdpip is incompatible with mno-fprnd option 146 | z[i][0] = __builtin_vsx_xsrdpip (z[i][1]); i++; | ^ I read thru the source code looking for other builtins with the same issue. I also created a script to compile all of the tests in gcc/testsuite/gcc.target/powerpc with the -mno-fprnd option to check for additional builtins that are incompatible with the -mno-fprnd option. These were the only two builtins that were identified as being incompatible with the -mno-fprnd option. Please let me know if the patch looks OK for mainline. Thanks. Carl Love ----------------------------------------------------------------------- rs6000: Add command line and builtin compatibility check PR/target 87583 gcc/ChangeLog 2020-03-10 Carl Love <cel@us.ibm.com> * gcc/config/rs6000/rs6000-c.c (altivec_resolve_overloaded_builtin): Add check for TARGET_FRND and VSX_BUILTIN_XSRDPIM, VSX_BUILTIN_XSRDPIP compatibility. --- gcc/config/rs6000/rs6000-c.c | 8 ++++++++ 1 file changed, 8 insertions(+) return NULL_TREE;