On Mar 2, 2021, David Edelsohn <dje....@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, -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Vim, Vi, Voltei pro Emacs -- GNUlius Caesar
