| Message ID | ora6rqx14n.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | add -mpowerpc-gpopt to options for sqrt insn on PowerPC | expand |
On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: > > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to > install? I'm sort of surprised that sqrt instruction would be available for the target but not enabled by default. Is this really the method that customers would use? It seems that it might be more appropriate to xfail or skip the test for PPC64 VxWorks. Thanks, David > > > From: Eric Botcazou <ebotcazou@adacore.com> > for gcc/testsuite/ChangeLog > > * lib/target-supports.exp (add_options_for_sqrt_insn): For > PowerPC targets, add -mpowerpc-gpopt option. > --- > gcc/testsuite/lib/target-supports.exp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp > index af46c77921482..29faeeba67945 100644 > --- a/gcc/testsuite/lib/target-supports.exp > +++ b/gcc/testsuite/lib/target-supports.exp > @@ -7598,6 +7598,9 @@ proc add_options_for_sqrt_insn { flags } { > if { [istarget arm*-*-*] } { > return [add_options_for_arm_vfp "$flags"] > } > + if { [istarget powerpc*-*-*] } { > + return "$flags -mpowerpc-gpopt" > + } > return $flags > } > > > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Vim, Vi, Voltei pro Emacs -- GNUlius Caesar
On Fri, Feb 26, 2021 at 12:31:16PM -0500, David Edelsohn wrote: > On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: > > > > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 > > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to > > install? > > I'm sort of surprised that sqrt instruction would be available for the > target but not enabled by default. Is this really the method that > customers would use? It seems that it might be more appropriate to > xfail or skip the test for PPC64 VxWorks. You should essentially never use -mpowerpc-gpopt, but instead use a -mcpu= that has it. You also of course whould not do that in run tests if the cpu does not support those insns. But, Alex, you say this avoids an ICE? You shouldn't avoid the ICE in that testcase then -- instead, fix it! (Or report it). Segher
On Sat, Feb 27, 2021 at 2:43 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Fri, Feb 26, 2021 at 12:31:16PM -0500, David Edelsohn wrote: > > On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: > > > > > > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 > > > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to > > > install? > > > > I'm sort of surprised that sqrt instruction would be available for the > > target but not enabled by default. Is this really the method that > > customers would use? It seems that it might be more appropriate to > > xfail or skip the test for PPC64 VxWorks. > > You should essentially never use -mpowerpc-gpopt, but instead use a > -mcpu= that has it. You also of course whould not do that in run tests > if the cpu does not support those insns. > > But, Alex, you say this avoids an ICE? You shouldn't avoid the ICE in > that testcase then -- instead, fix it! (Or report it). The testcase uses a .SQRT direct-internal function and guards itself with /* { dg-do compile { target sqrt_insn } } */ /* { dg-options "-fgimple -O2" } */ /* { dg-add-options sqrt_insn } */ if the power dg setup of sqrt_insn doesn't support this it should be adjusted. Using -mcpu=X for sqrt_insn is likely undesirable but for this testcase would work (it would make testing with, say, -mcpu=power4 not work). So I'd say as alternative to Alex patch you could adjust # Return 1 if the target supports hardware square root instructions. proc check_effective_target_sqrt_insn { } { return [check_cached_effective_target sqrt_insn { expr { [istarget i?86-*-*] || [istarget x86_64-*-*] || [istarget powerpc*-*-*] || [istarget aarch64*-*-*] || ([istarget arm*-*-*] && [check_effective_target_arm_vfp_ok]) || ([istarget s390*-*-*] && [check_effective_target_s390_vx]) || [istarget amdgcn-*-*] }}] } to only say yes in case the selected CPU supports sqrt. Richard. > > Segher
On Mon, Mar 01, 2021 at 09:46:29AM +0100, Richard Biener wrote: > On Sat, Feb 27, 2021 at 2:43 AM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > > On Fri, Feb 26, 2021 at 12:31:16PM -0500, David Edelsohn wrote: > > > On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: > > > > > > > > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 > > > > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to > > > > install? > > > > > > I'm sort of surprised that sqrt instruction would be available for the > > > target but not enabled by default. Is this really the method that > > > customers would use? It seems that it might be more appropriate to > > > xfail or skip the test for PPC64 VxWorks. > > > > You should essentially never use -mpowerpc-gpopt, but instead use a > > -mcpu= that has it. You also of course whould not do that in run tests > > if the cpu does not support those insns. > > > > But, Alex, you say this avoids an ICE? You shouldn't avoid the ICE in > > that testcase then -- instead, fix it! (Or report it). > > The testcase uses a .SQRT direct-internal function and guards itself with > > /* { dg-do compile { target sqrt_insn } } */ > /* { dg-options "-fgimple -O2" } */ > /* { dg-add-options sqrt_insn } */ > > if the power dg setup of sqrt_insn doesn't support this it should be adjusted. Yeah. It should test _ARCH_PPCSQ. I'll make a patch. > Using -mcpu=X for sqrt_insn is likely undesirable but for this testcase would > work (it would make testing with, say, -mcpu=power4 not work). Works differently on different dejagnu versions, which is why we have the -mdejagnu=cpu= kludge. > So I'd > say as alternative to Alex patch you could adjust > > # Return 1 if the target supports hardware square root instructions. > > proc check_effective_target_sqrt_insn { } { > return [check_cached_effective_target sqrt_insn { > expr { [istarget i?86-*-*] || [istarget x86_64-*-*] > || [istarget powerpc*-*-*] > || [istarget aarch64*-*-*] > || ([istarget arm*-*-*] && [check_effective_target_arm_vfp_ok]) > || ([istarget s390*-*-*] > && [check_effective_target_s390_vx]) > || [istarget amdgcn-*-*] }}] > } > > to only say yes in case the selected CPU supports sqrt. Right, replace || [istarget powerpc*-*-*] with || [check_effective_target_powerpc_sqrt] (and define that :-) ) Segher
On Feb 26, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Fri, Feb 26, 2021 at 12:31:16PM -0500, David Edelsohn wrote: >> On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: >> > >> > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 >> > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to >> > install? >> >> I'm sort of surprised that sqrt instruction would be available for the >> target but not enabled by default. Is this really the method that >> customers would use? It seems that it might be more appropriate to >> xfail or skip the test for PPC64 VxWorks. > You should essentially never use -mpowerpc-gpopt, but instead use a > -mcpu= that has it. You also of course whould not do that in run tests > if the cpu does not support those insns. I think the feedback is missing the point of the obvious bug that Eric's patch fixes. We have a dejagnu proc that should return any target-specific options necessary for a sqrt insn to be available: # Return any additional options to enable square root intructions. powerpc has an optional sqrt insn, but the option that enables it is not returned by that proc. That's a blatang bug in that proc. Do you see that, or do you have any sensible reasoning to share to support the position that the proc is NOT buggy? This proc is presumably only used in compile tests; it wouldn't make sense to assume an optional sqrt insn to be available on whatever hardware variant you happen to be using for testing. But the bug fixed by Eric's patch is pretty blatant, and I don't think it makes sense to reject this fix, and insist on a fix of another bug instead. That would just leave *this* bug in the dejagnu proc unfixed. Now, maybe a different flag is to be returned, but -mpowerpc-gpopt is the flag that enables sqrt with minimal disturbance of any other cpu selections in effect, so I believe that's the option that best serves the purpose of making the hardware sqrt insn available, regardless of whatever would be recommended to end users. > But, Alex, you say this avoids an ICE? You shouldn't avoid the ICE in > that testcase then -- instead, fix it! (Or report it). Orthogonal issue, IMHO. If you restate the response as "the proposed patch is fine as long as a bug report is on file for the error encountered when attempting to expand .<SQRT> when a sqrt intrinsic is not available", I can go along with it. But saying "we don't want to fix this bug, we want to fix another vaguely related bug *instead*", will make our stances mutually incompatible, and would likely result in my pursuing neither bug. While at that, if -mpowerpc-gpopt is not to be used, maybe you'll want to fix, or file a bug report, about the multiple tests gcc.target/powerpc/pr46728-*.c, that use it explicitly, and for the very purpose of enabling the fsqrt insn. Several of these are execution tests, so they fail on systems that don't offer fsqrt. I suppose these should mention *_sqrt_insn target requirements and add options. I haven't looked into how they interact, if at all.
Alex, dejagnu should not report that sqrt_insn is available on PowerPC in confirmations when it is not. The correct fix and the one suggested by Richard and Segher is to test for the availability of sqrt, not to assume that it exists in PowerPC. The test should not explicitly add -mpowerpc-gpopt because then it is not testing the configuration of the compiler and/or the configuration selected by the person running dejagnu, which makes the testcase moot. Thanks, David On Tue, Mar 2, 2021 at 6:14 AM Alexandre Oliva <oliva@adacore.com> wrote: > > On Feb 26, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > > On Fri, Feb 26, 2021 at 12:31:16PM -0500, David Edelsohn wrote: > >> On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: > >> > > >> > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 > >> > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to > >> > install? > >> > >> I'm sort of surprised that sqrt instruction would be available for the > >> target but not enabled by default. Is this really the method that > >> customers would use? It seems that it might be more appropriate to > >> xfail or skip the test for PPC64 VxWorks. > > > You should essentially never use -mpowerpc-gpopt, but instead use a > > -mcpu= that has it. You also of course whould not do that in run tests > > if the cpu does not support those insns. > > I think the feedback is missing the point of the obvious bug that Eric's > patch fixes. > > We have a dejagnu proc that should return any target-specific options > necessary for a sqrt insn to be available: > > # Return any additional options to enable square root intructions. > > powerpc has an optional sqrt insn, but the option that enables it is not > returned by that proc. That's a blatang bug in that proc. Do you see > that, or do you have any sensible reasoning to share to support the > position that the proc is NOT buggy? > > > This proc is presumably only used in compile tests; it wouldn't make > sense to assume an optional sqrt insn to be available on whatever > hardware variant you happen to be using for testing. > > But the bug fixed by Eric's patch is pretty blatant, and I don't think > it makes sense to reject this fix, and insist on a fix of another bug > instead. That would just leave *this* bug in the dejagnu proc unfixed. > > > Now, maybe a different flag is to be returned, but -mpowerpc-gpopt is > the flag that enables sqrt with minimal disturbance of any other cpu > selections in effect, so I believe that's the option that best serves > the purpose of making the hardware sqrt insn available, regardless of > whatever would be recommended to end users. > > > > But, Alex, you say this avoids an ICE? You shouldn't avoid the ICE in > > that testcase then -- instead, fix it! (Or report it). > > Orthogonal issue, IMHO. If you restate the response as "the proposed > patch is fine as long as a bug report is on file for the error > encountered when attempting to expand .<SQRT> when a sqrt intrinsic is > not available", I can go along with it. But saying "we don't want to > fix this bug, we want to fix another vaguely related bug *instead*", > will make our stances mutually incompatible, and would likely result in > my pursuing neither bug. > > > While at that, if -mpowerpc-gpopt is not to be used, maybe you'll want > to fix, or file a bug report, about the multiple tests > gcc.target/powerpc/pr46728-*.c, that use it explicitly, and for the very > purpose of enabling the fsqrt insn. Several of these are execution > tests, so they fail on systems that don't offer fsqrt. I suppose these > should mention *_sqrt_insn target requirements and add options. I > haven't looked into how they interact, if at all. > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > Vim, Vi, Voltei pro Emacs -- GNUlius Caesar
Alexandre Oliva <oliva@adacore.com> writes: > On Feb 26, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > >> On Fri, Feb 26, 2021 at 12:31:16PM -0500, David Edelsohn wrote: >>> On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: >>> > >>> > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 >>> > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to >>> > install? >>> >>> I'm sort of surprised that sqrt instruction would be available for the >>> target but not enabled by default. Is this really the method that >>> customers would use? It seems that it might be more appropriate to >>> xfail or skip the test for PPC64 VxWorks. > >> You should essentially never use -mpowerpc-gpopt, but instead use a >> -mcpu= that has it. You also of course whould not do that in run tests >> if the cpu does not support those insns. > > I think the feedback is missing the point of the obvious bug that Eric's > patch fixes. > > We have a dejagnu proc that should return any target-specific options > necessary for a sqrt insn to be available: > > # Return any additional options to enable square root intructions. Agreed FWIW. The intention of dg-add-options was to try to increase test coverage by supporting pairs of procs, one to answer “can I enable this feature?” and another to say “this is how to enable the feature”. The question isn't whether sqrt is already enabled, but whether it can be. Both proposed fixes are correct, in that they make the pairs of procs consistent. Changing add_options_for_sqrt_insn is more in the spirit of how this was supposed to work though. Thanks, Richard
On Tue, Mar 2, 2021 at 8:48 AM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Alexandre Oliva <oliva@adacore.com> writes: > > On Feb 26, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > > >> On Fri, Feb 26, 2021 at 12:31:16PM -0500, David Edelsohn wrote: > >>> On Fri, Feb 26, 2021 at 11:09 AM Alexandre Oliva <oliva@adacore.com> wrote: > >>> > > >>> > This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 > >>> > tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to > >>> > install? > >>> > >>> I'm sort of surprised that sqrt instruction would be available for the > >>> target but not enabled by default. Is this really the method that > >>> customers would use? It seems that it might be more appropriate to > >>> xfail or skip the test for PPC64 VxWorks. > > > >> You should essentially never use -mpowerpc-gpopt, but instead use a > >> -mcpu= that has it. You also of course whould not do that in run tests > >> if the cpu does not support those insns. > > > > I think the feedback is missing the point of the obvious bug that Eric's > > patch fixes. > > > > We have a dejagnu proc that should return any target-specific options > > necessary for a sqrt insn to be available: > > > > # Return any additional options to enable square root intructions. > > Agreed FWIW. The intention of dg-add-options was to try to increase > test coverage by supporting pairs of procs, one to answer “can I enable > this feature?” and another to say “this is how to enable the feature”. > The question isn't whether sqrt is already enabled, but whether it can be. > > Both proposed fixes are correct, in that they make the pairs of procs > consistent. Changing add_options_for_sqrt_insn is more in the spirit > of how this was supposed to work though. The procs are used in more than that one test. Maybe that one test was intended as a pair, but the use of the procs is more general than "does the target support sqrt instruction and the complementary options to enable the instruction". Thanks, David
On Tue, Mar 02, 2021 at 08:13:51AM -0300, Alexandre Oliva wrote: > On Feb 26, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > You should essentially never use -mpowerpc-gpopt, but instead use a > > -mcpu= that has it. You also of course whould not do that in run tests > > if the cpu does not support those insns. > > I think the feedback is missing the point of the obvious bug that Eric's > patch fixes. > > We have a dejagnu proc that should return any target-specific options > necessary for a sqrt insn to be available: > > # Return any additional options to enable square root intructions. > > powerpc has an optional sqrt insn, but the option that enables it is not > returned by that proc. That's a blatang bug in that proc. Do you see > that, or do you have any sensible reasoning to share to support the > position that the proc is NOT buggy? You often need more than one flag to enable this instruction, or it is simply impossible (if the CPU selected does not have it). You can need -mno-soft-float, -mpowerpc-gpopt, or it is simply impossible because your cpu does not implement these instructions (if any traditional floating point instructions, if any floating point at all!) > This proc is presumably only used in compile tests; it wouldn't make > sense to assume an optional sqrt insn to be available on whatever > hardware variant you happen to be using for testing. It also conflicts with other insns (in principle, I know no cpu that has some otther insns with this same opcode). > But the bug fixed by Eric's patch is pretty blatant, and I don't think > it makes sense to reject this fix, and insist on a fix of another bug > instead. That would just leave *this* bug in the dejagnu proc unfixed. The actual bug is in check_effective_target_sqrt_insn. I'm doing a patch, but I cannot say that in the PR because there is no PR. > > But, Alex, you say this avoids an ICE? You shouldn't avoid the ICE in > > that testcase then -- instead, fix it! (Or report it). > > Orthogonal issue, IMHO. If you restate the response as "the proposed > patch is fine as long as a bug report is on file for the error > encountered when attempting to expand .<SQRT> when a sqrt intrinsic is > not available", I can go along with it. But saying "we don't want to > fix this bug, we want to fix another vaguely related bug *instead*", > will make our stances mutually incompatible, and would likely result in > my pursuing neither bug. I am saying fixing an ICE is much more important than fixing a testsuite bug. I am not saying the latter should not be done; I am asking to please not sweep this under the rug. > While at that, if -mpowerpc-gpopt is not to be used, maybe you'll want > to fix, or file a bug report, about the multiple tests > gcc.target/powerpc/pr46728-*.c, that use it explicitly, and for the very > purpose of enabling the fsqrt insn. Several of these are execution > tests, so they fail on systems that don't offer fsqrt. I suppose these > should mention *_sqrt_insn target requirements and add options. I > haven't looked into how they interact, if at all. That there are broken tests does not mean we should break more. Yes, they should require a sqrt_insn effective target. Almost everything that is ever tested these days has these insns, and the insns are not usually disabled (which they always can be, -msoft-float disables the FP registers). So forcing gpopt on even *reduces* the coverage instead of increasing it! This is PR99352 now. Segher
On Mar 2, 2021, David Edelsohn <dje.gcc@gmail.com> wrote:
> The procs are used in more than that one test.
Err, are you looking at the trunk? In my tree, there are only two tests
that mention sqrt_insn, and it's two rather than one just because I
added the flag to another test myself, in a patch yet to be contributed.
Here's the patch that introduced the only use of the proc that is known
to me: https://gcc.gnu.org/legacy-ml/gcc-patches/2018-12/msg01204.html
As for the to-be-contributed patch, it adds the sqrt_insn feature option
to cdce3.c. Like gimplefe-28.c, a compile-only front-end test that
depends on the existence of a sqrt insn to do its job, cdce3.c is a
compile-only test for a middle-end shrink-wrap optimization, that is
only performed when there is a sqrt insn.
While we could hide the bug/missing feature in add_options_for_sqrt_insn
by constraining check_effective_target_sqrt_insn, the result would be
just reduced test coverage for powerpc builds that defaulted to
-mno-powerpc-gpopt. A downside without any upside.
Whereas if we fix the former proc to perform its documented function on
powerpc, namely return the options required to enable the fsqrt insn,
the end result is that, if the options do the job, we get those two
tests performed, whereas if they happen to be incompatible with other
settings to the point of raising an error, we (should?) skip the tests.
In my book, that's upsides without downsides.
Now, if the grounds for rejecting the patch was based on the
understanding that the proc was used elsewhere, with a different
function, I hope this demonstrates that this understanding was based on
incorrect information (that I may have hinted at myself; sorry if so),
and now that it's been corrected, I request the rejection of this
approach to be reconsidered.
I suppose the patch as-is might still be rejected, because it introduces
only -mpowerpc-gpopt, without -mno-soft-float. Since in some
configurations it may take both of them to enable the fsqrt insn, would
an alternate version that returned both be deemed acceptable instead?
Maybe we also want to document in the proc that these added options can
only be used in compile tests, because there's no expectation or
guarantee that the so-enabled feature is available in the target under
test. But AFAIK this has always been the case, and now I see and
confirm that a feature-option-adding call is always followed by a
feature-available-check call.
Thanks,
On Mar 2, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote:
> This is PR99352 now.
Thanks. I've filed PR99371 for the add_options_for_sqrt_insn
incompleteness, and PR99372 for the gimplefe-28.c ICE.
hi! On Wed, Mar 03, 2021 at 04:22:29PM -0300, Alexandre Oliva wrote: > While we could hide the bug/missing feature in add_options_for_sqrt_insn > by constraining check_effective_target_sqrt_insn, the result would be > just reduced test coverage for powerpc builds that defaulted to > -mno-powerpc-gpopt. A downside without any upside. You just *cannot* use add_options_for_sqrt_insn usefully ever (on our configurations): unless you override which -mcpu= is used, and whether hard float is enabled, you cannot depend on having classic float (and you need to disable VSX or detect x[sv]sqrtdp etc. as well as fsqrt). > Whereas if we fix the former proc to perform its documented function on > powerpc, namely return the options required to enable the fsqrt insn, But in all configurations where you *can*, it already is, unless it is "manually" disabled by using -msoft-float. Which a) almost no one does, and b) overriding that causes *less* testing, not *more*. Anyway, this is PR99352 now, and I have a patch (that works with -msoft-float as well, etc.) Will commit later today. Segher
On Wed, Mar 03, 2021 at 04:45:23PM -0300, Alexandre Oliva wrote: > On Mar 2, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > > This is PR99352 now. > > Thanks. I've filed PR99371 for the add_options_for_sqrt_insn > incompleteness, As I said before, I don't agree with that. We do no longer support enabling separate features / insns, because that requires exponential testing effort, and an exponential number of backend patterns as well -- and very often some are missed. And on all systems where these insns exist, they are enabled by default. If a user disabled it, we should *not* reenable it, that reduces testing surface. > and PR99372 for the gimplefe-28.c ICE. This is fixed trivially by the PR99352 patch as far as I can see. Please verify (I'll post it later today). Segher
On Mar 3, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Wed, Mar 03, 2021 at 04:45:23PM -0300, Alexandre Oliva wrote: >> On Mar 2, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> >> > This is PR99352 now. >> >> Thanks. I've filed PR99371 for the add_options_for_sqrt_insn >> incompleteness, > As I said before, I don't agree with that. Maybe you'll change of mind if you try to make sense of why the proc has, since its inception, added vfp options to enable sqrt on arm, regardless of whether vfp is available with the processor being tested, and realize this is not different from the case of powerpc. > If a user disabled it, we should *not* reenable it, that reduces > testing surface. It's skipping the test, as the change you propose, that reduces testing surface, when testing only a configuration that ends up skipping it. Now, if you're testing multiple combinations, skipping or running does *not* change the test surface. So rejecting Eric's patch makes for a no-op in one case, and a reduction in another. Whereas installing it makes for a no-op in one case, and an increase in another. Please explain how you came to the conclusion that this amounts to reducing hte test surface. Something appears to be amiss in that reasoning. >> and PR99372 for the gimplefe-28.c ICE. > This is fixed trivially by the PR99352 patch as far as I can see. If your patch also deals with the ICE that appears with the options named in PR99372, great. > Please verify (I'll post it later today). Please Cc me so I don't miss it, thanks,
Hi! On Wed, Mar 03, 2021 at 06:07:29PM -0300, Alexandre Oliva wrote: > On Mar 3, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > It's skipping the test, as the change you propose, that reduces testing > surface, when testing only a configuration that ends up skipping it. Not at all. There are very many more configurations that have floating point disabled then that there are configurations with FP enabled but without square root insns (almost no one targets G4 anymore, it is over twenty years old). In any case, the existing code did not do two necessary checks. I corrected that. > > This is fixed trivially by the PR99352 patch as far as I can see. > > If your patch also deals with the ICE that appears with the options > named in PR99372, great. It does afaics. Please check? Segher
On Mar 9, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: > Hi! > On Wed, Mar 03, 2021 at 06:07:29PM -0300, Alexandre Oliva wrote: >> On Mar 3, 2021, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> It's skipping the test, as the change you propose, that reduces testing >> surface, when testing only a configuration that ends up skipping it. > Not at all. There are very many more configurations that have floating > point disabled then that there are configurations with FP enabled but > without square root insns (almost no one targets G4 anymore, it is over > twenty years old). Your change causes the test to be skipped. How does that not reduce the testing surface? > In any case, the existing code did not do two necessary checks. I > corrected that. I agree your change is correct, and a step in the right direction, it's just not sufficient to address all of the problems that have been brought up in this interaction. >> > This is fixed trivially by the PR99352 patch as far as I can see. >> If your patch also deals with the ICE that appears with the options >> named in PR99372, great. > It does afaics. Please check? I still get an ICE when I compile the testcase with the options mentioned in the testcase. Don't you?
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index af46c77921482..29faeeba67945 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -7598,6 +7598,9 @@ proc add_options_for_sqrt_insn { flags } { if { [istarget arm*-*-*] } { return [add_options_for_arm_vfp "$flags"] } + if { [istarget powerpc*-*-*] } { + return "$flags -mpowerpc-gpopt" + } return $flags }
This patch avoids an ICE in gimplefe-28.c, in our ppc64-vxworks7r2 tests. Tested on x86_64-linux-gnu, and on the affected platform. Ok to install? From: Eric Botcazou <ebotcazou@adacore.com> for gcc/testsuite/ChangeLog * lib/target-supports.exp (add_options_for_sqrt_insn): For PowerPC targets, add -mpowerpc-gpopt option. --- gcc/testsuite/lib/target-supports.exp | 3 +++ 1 file changed, 3 insertions(+)