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
