| Message ID | 90085edf3a0b40f06d49740b87e2b4bf9e2a3fc0.1551458843.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Add -mdejagnu-cpu= | expand |
On Fri, Mar 01, 2019 at 05:02:58PM +0000, Segher Boessenkool wrote: > This adds an option -mdejagnu-cpu=. This option simply overrides what > is given in -mcpu=. The reason for this is that with older versions > of DejaGnu the value given in the RUNTESTFLAGS will override the value > a testcase wants to have. Ugh, that is ugly. Can't we just detect the old dejagnu and override whatever tcl method is responsible for that? Or just document higher minimum dejagnu version requirement? > Ill commit this patch with the first changelog, and also the result of > these two one-liners with the second changelog: > > perl -ni -e 'print unless /dg-skip-if "do not override -mcpu"/' \ > $(find gcc/testsuite/gcc.target/powerpc/ -type f) > perl -pi -e 's/(dg-options.*)-mcpu=/\1-mdejagnu-cpu=/' \ > $(find gcc/testsuite/gcc.target/powerpc/ -type f) > > > Segher > > > 2019-03-01 Segher Boessenkool <segher@kernel.crashing.org> > > * config/rs6000/rs6000.c (rs6000_option_override_internal): If > rs6000_dejagnu_cpu_index is set, use that to override rs6000_cpu_index. > * config/rs6000/rs6000.opt (mdejagnu-cpu=): New option. > > > 2019-03-01 Segher Boessenkool <segher@kernel.crashing.org> > > gcc/testsuite/ > * gcc.target/powerpc/ throughout: Delete dg-skip-if "do not override > -mcpu". Use -mdejagnu-cpu= in dg-options instead of -mcpu=. Jakub
On Fri, Mar 01, 2019 at 06:07:57PM +0100, Jakub Jelinek wrote: > On Fri, Mar 01, 2019 at 05:02:58PM +0000, Segher Boessenkool wrote: > > This adds an option -mdejagnu-cpu=. This option simply overrides what > > is given in -mcpu=. The reason for this is that with older versions > > of DejaGnu the value given in the RUNTESTFLAGS will override the value > > a testcase wants to have. > > Ugh, that is ugly. Hey, it's a nice improvement over what we have had for years already. > Can't we just detect the old dejagnu and override > whatever tcl method is responsible for that? Not sure. We probably could. But are the new semantics actually better? Sometimes you want the testcase flags to override the RUNTESTFLAGS, but just as often you don't. So now older dejagnu versions have one behaviour (and we will see those for very many years still, certainly on Power, where we mostly use slow^Wold^Wenterprise distros), and newer dejagnu versions have another behaviour, some things break with the old, some things break with the new, and it's all a big mess. Having separate flags for the testcases that say "override the RUNTESTFLAGS" gives the best of both worlds. Even with with new dejagnu, where normally the RUNTESTFLAGS always wins. > Or just document higher minimum dejagnu version requirement? Maybe in ten years time? Or *maybe* in five. Some systems still ship with 1.5.1... (On powerpc we can get away with this because testcases do not (well, should not) use -m32/-m64, and we only use -mcpu in the RUNTESTFLAGS otherwise). Segher
On Fri, Mar 01, 2019 at 12:00:50PM -0600, Segher Boessenkool wrote: > On Fri, Mar 01, 2019 at 06:07:57PM +0100, Jakub Jelinek wrote: > > On Fri, Mar 01, 2019 at 05:02:58PM +0000, Segher Boessenkool wrote: > > > This adds an option -mdejagnu-cpu=. This option simply overrides what > > > is given in -mcpu=. The reason for this is that with older versions > > > of DejaGnu the value given in the RUNTESTFLAGS will override the value > > > a testcase wants to have. > > > > Ugh, that is ugly. > > Hey, it's a nice improvement over what we have had for years already. Even if we don't document the new option, I'm sure some users will start using it, report bugs for it, ... > > Can't we just detect the old dejagnu and override > > whatever tcl method is responsible for that? > > Not sure. We probably could. But are the new semantics actually > better? Sometimes you want the testcase flags to override the RUNTESTFLAGS, > but just as often you don't. So now older dejagnu versions have one > behaviour (and we will see those for very many years still, certainly on > Power, where we mostly use slow^Wold^Wenterprise distros), and newer dejagnu > versions have another behaviour, some things break with the old, some things > break with the new, and it's all a big mess. CCing Jonathan here, as he has I think dealt with this a lot in the libstdc++ testsuite. I believe the new dejagnu behavior is considered the desirable/right one, I fail to see when the old behavior would be wanted. If a testcase wants to override something, it should be able to. > > Having separate flags for the testcases that say "override the RUNTESTFLAGS" > gives the best of both worlds. Even with with new dejagnu, where normally > the RUNTESTFLAGS always wins. I thought new dejagnu behavior is RUNTESTFLAGS are placed early on the command line and dg-options follow. Jakub
On Fri, Mar 01, 2019 at 07:09:05PM +0100, Jakub Jelinek wrote: > > Having separate flags for the testcases that say "override the RUNTESTFLAGS" > > gives the best of both worlds. Even with with new dejagnu, where normally > > the RUNTESTFLAGS always wins. > > I thought new dejagnu behavior is RUNTESTFLAGS are placed early on the > command line and dg-options follow. Yes I wrote that backwards, sorry. Segher
On Fri, Mar 01, 2019 at 07:09:05PM +0100, Jakub Jelinek wrote: > > > Can't we just detect the old dejagnu and override > > > whatever tcl method is responsible for that? > > > > Not sure. We probably could. But are the new semantics actually > > better? Sometimes you want the testcase flags to override the RUNTESTFLAGS, > > but just as often you don't. So now older dejagnu versions have one > > behaviour (and we will see those for very many years still, certainly on > > Power, where we mostly use slow^Wold^Wenterprise distros), and newer dejagnu > > versions have another behaviour, some things break with the old, some things > > break with the new, and it's all a big mess. > > CCing Jonathan here, as he has I think dealt with this a lot in the > libstdc++ testsuite. > > I believe the new dejagnu behavior is considered the desirable/right one, I > fail to see when the old behavior would be wanted. If a testcase wants to > override something, it should be able to. > > > > Having separate flags for the testcases that say "override the RUNTESTFLAGS" > > gives the best of both worlds. Even with with new dejagnu, where normally > > the RUNTESTFLAGS always wins. > > I thought new dejagnu behavior is RUNTESTFLAGS are placed early on the > command line and dg-options follow. We are talking about the http://git.savannah.gnu.org/cgit/dejagnu.git/commit/?id=5256bd82343000c76bc0e48139003f90b6184347 change, right? If we choose to require dejagnu with that fix, shouldn't be that hard to apply that one-liner patch if whole dejagnu can't be upgraded. Or just like for libstdc++, document that using --target_board multilib options with older dejagnu is unsupported or has certain limitations. Jakub
On Fri, Mar 01, 2019 at 07:33:27PM +0100, Jakub Jelinek wrote: > We are talking about the > http://git.savannah.gnu.org/cgit/dejagnu.git/commit/?id=5256bd82343000c76bc0e48139003f90b6184347 > change, right? That's the patch I think, yes. One thing I didn't mention is my patch fixed some ten failures, mostly code that set things like /* { dg-options "-maltivec -mcpu=power8" } */ /* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */ which does not work as you might expect. (My patch removed all such dg-skip-if lines). Another reason is that we currently use -mpower8-vector (etc.) to select power8, not just the power8 vector extensions, which should not even _have_ a user-accessible option. To clean up those options we need to make -mcpu= in the testsuite work better. It's all by no means perfect. But it's a clear improvement, in my mind. Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index b1249bc..b489bef 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -3869,6 +3869,9 @@ rs6000_option_override_internal (bool global_init_p) /* Don't override by the processor default if given explicitly. */ set_masks &= ~rs6000_isa_flags_explicit; + if (global_init_p && rs6000_dejagnu_cpu_index >= 0) + rs6000_cpu_index = rs6000_dejagnu_cpu_index; + /* Process the -mcpu=<xxx> and -mtune=<xxx> argument. If the user changed the cpu in a target attribute or pragma, but did not specify a tuning option, use the cpu for the tuning option rather than the option specified diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt index 2e90bf3..f4b5c91 100644 --- a/gcc/config/rs6000/rs6000.opt +++ b/gcc/config/rs6000/rs6000.opt @@ -385,6 +385,13 @@ mtune= Target RejectNegative Joined Var(rs6000_tune_index) Init(-1) Enum(rs6000_cpu_opt_value) Save -mtune= Schedule code for given CPU. +; Only for use in the testsuite. This simply overrides -mcpu=. With older +; versions of Dejagnu the command line arguments you set in RUNTESTFLAGS +; override those set in the testcases; with this option, the testcase will +; always win. +mdejagnu-cpu= +Target Undocumented RejectNegative Joined Var(rs6000_dejagnu_cpu_index) Init(-1) Enum(rs6000_cpu_opt_value) Save + mtraceback= Target RejectNegative Joined Enum(rs6000_traceback_type) Var(rs6000_traceback) -mtraceback=[full,part,no] Select type of traceback table.