On Tue, Mar 02, 2021 at 08:13:51AM -0300, Alexandre Oliva wrote: > On Feb 26, 2021, Segher Boessenkool <seg...@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
