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 <ol...@adacore.com> wrote: > > On Feb 26, 2021, Segher Boessenkool <seg...@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 <ol...@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
