Re: [PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Tue, Jan 25, 2022 at 01:28:29PM -0300, Raoni Fassina Firmino wrote:
> Below is a patch to do just that. In preliminary tests it seems to work.
> What do you think aboud it Jakub?
Ok for trunk.
> > These days the usual way of doing this is through
> > maybe_expand_insn and create_{output,input}_operand before that.
>
> I looked into maybe_expand_insn, if I undestood correctly in my reading
> of maybe_expand_insn I could remove mostly if not all code in
> expand_builtin_feclear_feraise_except with it, even the validate_arglist
> part in the beginning?
validate_arglist should be still performed. But maybe_expand_insn +
create_*_operand will take care of checking the predicates etc.
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> @@ -2598,6 +2598,9 @@ expand_builtin_feclear_feraise_except (tree exp, rtx
> target,
>if (icode == CODE_FOR_nothing)
> return NULL_RTX;
>
> + if (!(*insn_data[icode].operand[1].predicate) (op0, GET_MODE(op0)))
> +return NULL_RTX;
> +
>if (target == 0
>|| GET_MODE (target) != target_mode
>|| !(*insn_data[icode].operand[0].predicate) (target, target_mode))
> --
> 2.34.1
Jakub
Re: [PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Mon, Jan 24, 2022 at 11:35:47PM +0100, AL gcc-patches wrote:
> On Mon, Jan 24, 2022 at 06:24:11PM -0300, Raoni Fassina Firmino wrote:
> > On Mon, Jan 24, 2022 at 02:29:39PM -0600, Bill Schmidt wrote:
> > > Adding the patch author for his information.
> >
> > Thanks Bill.
> >
> > > On 1/24/22 2:26 PM, Jakub Jelinek via Gcc-patches wrote:
> > > > expand_builtin_feclear_feraise_except doesn't check if op0 matches
> > > > the predicate of operands[1], the backend requires const_int_operand,
Below is a patch to do just that. In preliminary tests it seems to work.
What do you think aboud it Jakub?
> > > > but because the call isn't done with a constant integer:
> > > > feraiseexcept (t == LLONG_MIN ? FE_INEXACT : FE_INVALID);
> > > > op0 is a REG.
> > > > If CONST_INT is what is expected on all targets, then it should punt if
> > > > op0 isn't one, otherwise it should the predicate.
> >
> > My expectation was for backend expanders to be free to choose when they
> > expand, to be able to fail and cleanly fallback to libc call, and not
> > enforce one specific constrains to all targets.
> >
> > I will look into it ASAP and see what can be done.
> > Thanks for the feedback.
>
> These days the usual way of doing this is through
> maybe_expand_insn and create_{output,input}_operand before that.
I looked into maybe_expand_insn, if I undestood correctly in my reading
of maybe_expand_insn I could remove mostly if not all code in
expand_builtin_feclear_feraise_except with it, even the validate_arglist
part in the beginning?
o/
Raoni
---
gcc/builtins.cc | 3 +++
1 file changed, 3 insertions(+)
diff --git a/gcc/builtins.cc b/gcc/builtins.cc
index e84208035dab..88e689993563 100644
--- a/gcc/builtins.cc
+++ b/gcc/builtins.cc
@@ -2598,6 +2598,9 @@ expand_builtin_feclear_feraise_except (tree exp, rtx
target,
if (icode == CODE_FOR_nothing)
return NULL_RTX;
+ if (!(*insn_data[icode].operand[1].predicate) (op0, GET_MODE(op0)))
+return NULL_RTX;
+
if (target == 0
|| GET_MODE (target) != target_mode
|| !(*insn_data[icode].operand[0].predicate) (target, target_mode))
--
2.34.1
Re: [PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Mon, Jan 24, 2022 at 06:24:11PM -0300, Raoni Fassina Firmino wrote:
> On Mon, Jan 24, 2022 at 02:29:39PM -0600, Bill Schmidt wrote:
> > Adding the patch author for his information.
>
> Thanks Bill.
>
> > On 1/24/22 2:26 PM, Jakub Jelinek via Gcc-patches wrote:
> > > expand_builtin_feclear_feraise_except doesn't check if op0 matches
> > > the predicate of operands[1], the backend requires const_int_operand,
> > > but because the call isn't done with a constant integer:
> > > feraiseexcept (t == LLONG_MIN ? FE_INEXACT : FE_INVALID);
> > > op0 is a REG.
> > > If CONST_INT is what is expected on all targets, then it should punt if
> > > op0 isn't one, otherwise it should the predicate.
>
> My expectation was for backend expanders to be free to choose when they
> expand, to be able to fail and cleanly fallback to libc call, and not
> enforce one specific constrains to all targets.
>
> I will look into it ASAP and see what can be done.
> Thanks for the feedback.
These days the usual way of doing this is through
maybe_expand_insn and create_{output,input}_operand before that.
Jakub
Re: [PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Mon, Jan 24, 2022 at 02:29:39PM -0600, Bill Schmidt wrote: > Adding the patch author for his information. Thanks Bill. > On 1/24/22 2:26 PM, Jakub Jelinek via Gcc-patches wrote: > > expand_builtin_feclear_feraise_except doesn't check if op0 matches > > the predicate of operands[1], the backend requires const_int_operand, > > but because the call isn't done with a constant integer: > > feraiseexcept (t == LLONG_MIN ? FE_INEXACT : FE_INVALID); > > op0 is a REG. > > If CONST_INT is what is expected on all targets, then it should punt if > > op0 isn't one, otherwise it should the predicate. My expectation was for backend expanders to be free to choose when they expand, to be able to fail and cleanly fallback to libc call, and not enforce one specific constrains to all targets. I will look into it ASAP and see what can be done. Thanks for the feedback. o/ Raoni
Re: [PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Adding the patch author for his information. Thanks, Bill On 1/24/22 2:26 PM, Jakub Jelinek via Gcc-patches wrote: > On Mon, Jan 24, 2022 at 08:55:37AM -0600, Segher Boessenkool wrote: >> Hi! >> >> On Thu, Jan 13, 2022 at 02:08:53PM -0300, Raoni Fassina Firmino wrote: >>> Changes since v8[8]: >>> - Refactored and expanded builtin-feclearexcept-feraiseexcept-2.c >>> testcase: >>> + Use a macro to avoid extended repetition of the core test code. >>> + Expanded the test code to check builtins return code. >>> + Added more tests to test all valid (standard) exceptions input >> This is okay for trunk (Jeff already approved the generic parts). >> Thanks! > This breaks bootstrap with --enable-checking=rtl, e.g. while compiling > libquadmath/math/llrintq.c > #0 internal_error (gmsgid=0x131bb1e0 "RTL check: expected code '%s', have > '%s' in %s, at %s:%d") at ../../gcc/diagnostic.cc:1938 > #1 0x113a0e94 in rtl_check_failed_code1 (r=0x3fffaf4a24a8, > code=CONST_INT, file=0x13400018 "../../gcc/config/rs6000/rs6000.md", > line=7010, > func=0x13409298 > "gen_feraiseexceptsi") at ../../gcc/rtl.cc:918 > #2 0x125154e8 in gen_feraiseexceptsi (operand0=0x3fffaf4a3720, > operand1=0x3fffaf4a24a8) at ../../gcc/config/rs6000/rs6000.md:7010 > #3 0x108badf4 in insn_gen_fn::operator() > (this=0x138ee440 ) at ../../gcc/recog.h:407 > #4 0x10890b1c in expand_builtin_feclear_feraise_except > (exp=0x3fffaf3041a0, target=0x3fffaf4a3720, target_mode=E_SImode, > op_optab=feraiseexcept_optab) > at ../../gcc/builtins.cc:2606 > #5 0x108a6f74 in expand_builtin (exp=0x3fffaf3041a0, > target=0x3fffaf100490, subtarget=0x0, mode=E_VOIDmode, ignore=1) at > ../../gcc/builtins.cc:7130 > #6 0x10c01770 in expand_expr_real_1 (exp=0x3fffaf3041a0, target=0x0, > tmode=E_VOIDmode, modifier=EXPAND_NORMAL, alt_rtl=0x0, > inner_reference_p=false) > at ../../gcc/expr.cc:11536 > #7 0x10bf0604 in expand_expr_real (exp=0x3fffaf3041a0, > target=0x3fffaf100490, tmode=E_VOIDmode, modifier=EXPAND_NORMAL, alt_rtl=0x0, > inner_reference_p=false) > at ../../gcc/expr.cc:8737 > #8 0x108ffa00 in expand_expr (exp=0x3fffaf3041a0, > target=0x3fffaf100490, mode=E_VOIDmode, modifier=EXPAND_NORMAL) at > ../../gcc/expr.h:301 > #9 0x1090c934 in expand_call_stmt (stmt=0x3fffaf1314d0) at > ../../gcc/cfgexpand.cc:2831 > #10 0x10911e18 in expand_gimple_stmt_1 (stmt=0x3fffaf1314d0) at > ../../gcc/cfgexpand.cc:3864 > #11 0x10912730 in expand_gimple_stmt (stmt=0x3fffaf1314d0) at > ../../gcc/cfgexpand.cc:4028 > #12 0x1091ecb0 in expand_gimple_basic_block (bb=0x3fffaf190c98, > disable_tail_calls=false) at ../../gcc/cfgexpand.cc:6069 > #13 0x10921be8 in (anonymous namespace)::pass_expand::execute > (this=0x13ab0d40, fun=0x3fffaf0c0c38) at ../../gcc/cfgexpand.cc:6795 > #14 0x11216ea4 in execute_one_pass (pass=0x13ab0d40) at > ../../gcc/passes.cc:2637 > #15 0x112173d8 in execute_pass_list_1 (pass=0x13ab0d40) at > ../../gcc/passes.cc:2737 > #16 0x112174b0 in execute_pass_list (fn=0x3fffaf0c0c38, > pass=0x13aac8c0) at ../../gcc/passes.cc:2748 > #17 0x109b4e4c in cgraph_node::expand (this=0x3fffaf151760) at > ../../gcc/cgraphunit.cc:1834 > #18 0x109b5844 in expand_all_functions () at > ../../gcc/cgraphunit.cc:1998 > #19 0x109b67d0 in symbol_table::compile (this=0x3fffaf0d) at > ../../gcc/cgraphunit.cc:2348 > #20 0x109b6f40 in symbol_table::finalize_compilation_unit > (this=0x3fffaf0d) at ../../gcc/cgraphunit.cc:2529 > #21 0x114f10f4 in compile_file () at ../../gcc/toplev.cc:479 > #22 0x114f6204 in do_compile (no_backend=false) at > ../../gcc/toplev.cc:2158 > #23 0x114f68d0 in toplev::main (this=0x3fffeb64, argc=45, > argv=0x3fffef98) at ../../gcc/toplev.cc:2310 > #24 0x12f97a6c in main (argc=45, argv=0x3fffef98) at > ../../gcc/main.cc:39 > > expand_builtin_feclear_feraise_except doesn't check if op0 matches > the predicate of operands[1], the backend requires const_int_operand, > but because the call isn't done with a constant integer: > feraiseexcept (t == LLONG_MIN ? FE_INEXACT : FE_INVALID); > op0 is a REG. > If CONST_INT is what is expected on all targets, then it should punt if > op0 isn't one, otherwise it should the predicate. > > Jakub >
Re: [PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
On Mon, Jan 24, 2022 at 08:55:37AM -0600, Segher Boessenkool wrote: > Hi! > > On Thu, Jan 13, 2022 at 02:08:53PM -0300, Raoni Fassina Firmino wrote: > > Changes since v8[8]: > > - Refactored and expanded builtin-feclearexcept-feraiseexcept-2.c > > testcase: > > + Use a macro to avoid extended repetition of the core test code. > > + Expanded the test code to check builtins return code. > > + Added more tests to test all valid (standard) exceptions input > > This is okay for trunk (Jeff already approved the generic parts). > Thanks! This breaks bootstrap with --enable-checking=rtl, e.g. while compiling libquadmath/math/llrintq.c #0 internal_error (gmsgid=0x131bb1e0 "RTL check: expected code '%s', have '%s' in %s, at %s:%d") at ../../gcc/diagnostic.cc:1938 #1 0x113a0e94 in rtl_check_failed_code1 (r=0x3fffaf4a24a8, code=CONST_INT, file=0x13400018 "../../gcc/config/rs6000/rs6000.md", line=7010, func=0x13409298 "gen_feraiseexceptsi") at ../../gcc/rtl.cc:918 #2 0x125154e8 in gen_feraiseexceptsi (operand0=0x3fffaf4a3720, operand1=0x3fffaf4a24a8) at ../../gcc/config/rs6000/rs6000.md:7010 #3 0x108badf4 in insn_gen_fn::operator() (this=0x138ee440 ) at ../../gcc/recog.h:407 #4 0x10890b1c in expand_builtin_feclear_feraise_except (exp=0x3fffaf3041a0, target=0x3fffaf4a3720, target_mode=E_SImode, op_optab=feraiseexcept_optab) at ../../gcc/builtins.cc:2606 #5 0x108a6f74 in expand_builtin (exp=0x3fffaf3041a0, target=0x3fffaf100490, subtarget=0x0, mode=E_VOIDmode, ignore=1) at ../../gcc/builtins.cc:7130 #6 0x10c01770 in expand_expr_real_1 (exp=0x3fffaf3041a0, target=0x0, tmode=E_VOIDmode, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=false) at ../../gcc/expr.cc:11536 #7 0x10bf0604 in expand_expr_real (exp=0x3fffaf3041a0, target=0x3fffaf100490, tmode=E_VOIDmode, modifier=EXPAND_NORMAL, alt_rtl=0x0, inner_reference_p=false) at ../../gcc/expr.cc:8737 #8 0x108ffa00 in expand_expr (exp=0x3fffaf3041a0, target=0x3fffaf100490, mode=E_VOIDmode, modifier=EXPAND_NORMAL) at ../../gcc/expr.h:301 #9 0x1090c934 in expand_call_stmt (stmt=0x3fffaf1314d0) at ../../gcc/cfgexpand.cc:2831 #10 0x10911e18 in expand_gimple_stmt_1 (stmt=0x3fffaf1314d0) at ../../gcc/cfgexpand.cc:3864 #11 0x10912730 in expand_gimple_stmt (stmt=0x3fffaf1314d0) at ../../gcc/cfgexpand.cc:4028 #12 0x1091ecb0 in expand_gimple_basic_block (bb=0x3fffaf190c98, disable_tail_calls=false) at ../../gcc/cfgexpand.cc:6069 #13 0x10921be8 in (anonymous namespace)::pass_expand::execute (this=0x13ab0d40, fun=0x3fffaf0c0c38) at ../../gcc/cfgexpand.cc:6795 #14 0x11216ea4 in execute_one_pass (pass=0x13ab0d40) at ../../gcc/passes.cc:2637 #15 0x112173d8 in execute_pass_list_1 (pass=0x13ab0d40) at ../../gcc/passes.cc:2737 #16 0x112174b0 in execute_pass_list (fn=0x3fffaf0c0c38, pass=0x13aac8c0) at ../../gcc/passes.cc:2748 #17 0x109b4e4c in cgraph_node::expand (this=0x3fffaf151760) at ../../gcc/cgraphunit.cc:1834 #18 0x109b5844 in expand_all_functions () at ../../gcc/cgraphunit.cc:1998 #19 0x109b67d0 in symbol_table::compile (this=0x3fffaf0d) at ../../gcc/cgraphunit.cc:2348 #20 0x109b6f40 in symbol_table::finalize_compilation_unit (this=0x3fffaf0d) at ../../gcc/cgraphunit.cc:2529 #21 0x114f10f4 in compile_file () at ../../gcc/toplev.cc:479 #22 0x114f6204 in do_compile (no_backend=false) at ../../gcc/toplev.cc:2158 #23 0x114f68d0 in toplev::main (this=0x3fffeb64, argc=45, argv=0x3fffef98) at ../../gcc/toplev.cc:2310 #24 0x12f97a6c in main (argc=45, argv=0x3fffef98) at ../../gcc/main.cc:39 expand_builtin_feclear_feraise_except doesn't check if op0 matches the predicate of operands[1], the backend requires const_int_operand, but because the call isn't done with a constant integer: feraiseexcept (t == LLONG_MIN ? FE_INEXACT : FE_INVALID); op0 is a REG. If CONST_INT is what is expected on all targets, then it should punt if op0 isn't one, otherwise it should the predicate. Jakub
Re: [PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Hi! On Thu, Jan 13, 2022 at 02:08:53PM -0300, Raoni Fassina Firmino wrote: > Changes since v8[8]: > - Refactored and expanded builtin-feclearexcept-feraiseexcept-2.c > testcase: > + Use a macro to avoid extended repetition of the core test code. > + Expanded the test code to check builtins return code. > + Added more tests to test all valid (standard) exceptions input This is okay for trunk (Jeff already approved the generic parts). Thanks! Segher
[PATCH v9] rtl: builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
Changes since v8[8]: - Refactored and expanded builtin-feclearexcept-feraiseexcept-2.c testcase: + Use a macro to avoid extended repetition of the core test code. + Expanded the test code to check builtins return code. + Added more tests to test all valid (standard) exceptions input combinations. + Updated the header comment to explain why the input must be passed as constants. Changes since v7[7]: - Fixed an array indexing bug on fegeround testcase. - Fixed typos and spelling mistakes spread trouout the added comments. - Reworded header comment/description for fegetround expander. - Fixed changelog in the commit message. This is an update to v8, based on the same review from Seguer to expand the test coverage for feclearexcept and feraiseexcept (That is also why I am keeping the v8 changelog here, since v8 had no reviews). Two things to point out is: 1) The use of a macro there instead of a function, unfortunately the builtins (for rs6000) only expand when the input is a constant, so a macro is the way to go, and for the same reason 2) I wanted to simplify the way to test all combinations of input, but I could not think in a way without making some macro magics that would be way less readable than listing all combinations by hand. Tested on top of master (02a8a01bf396e009bfc31e1104c315fd403b4cca) on the following plataforms with no regression: - powerpc64le-linux-gnu (Power 9) - powerpc64le-linux-gnu (Power 8) - powerpc64-linux-gnu (Power 9, with 32 and 64 bits tests) Documentation changes tested on x86_64-redhat-linux. == I'm repeating the "changelog" from past versions here for convenience: Changes since v6[6] and v5[5]: - Based this version on the v5 one. - Reworked all builtins back to the way they are in v5 and added the following changes: + Added a test to target libc, only expanding with glibc as the target libc. + Updated all three expanders header comment to reflect the added behavior (fegetround got a full header as it had none). + Added extra documentation for the builtins on doc/extend.texi, similar to v6 version, but only the introductory paragraph, without a dedicated entry for each, since now they behavior and signature match the C99 ones. - Changed the description for the return operand in the RTL template of the fegetround expander. Using "(set )", the same way as rs6000_mffsl expander (this change was taken from v6). - Updated the commit message mentioning the target libc restriction and updated changelog. Changes since v5[5]: - Reworked all builtins to accept the FE_* macros as parameters and so be agnostic to libc implementations. Largely based of fpclassify. To that end, there is some new files changed: + Change the argument list for the builtins declarations in builtins.def + Added new types in builtin-types.def to use in the buitins declarations. + Added extra documentation for the builtins on doc/extend.texi, similar to fpclassify. - Updated doc/md.texi documentation with the new optab behaviors. - Updated comments to the expanders and expand handlers to try to explain whats is going on. - Changed the description for the return operand in the RTL template of the fegetround expander. Using "(set )", the same way as rs6000_mffsl expander. - Updated testcases with helper macros with the new argument list. Changes since v4[4]: - Fixed more spelling and code style. - Add more clarification on comments for feraiseexcept and feclearexcept expands; Changes since v3[3]: - Fixed fegetround bug on powerpc64 (big endian) that Segher spotted; Changes since v2[2]: - Added documentation for the new optabs; - Remove use of non portable __builtin_clz; - Changed feclearexcept and feraiseexcept to accept all 4 valid flags at the same time and added more test for that case; - Extended feclearexcept and feraiseexcept testcases to match accepting multiple flags; - Fixed builtin-feclearexcept-feraiseexcept-2.c testcase comparison after feclearexcept tests; - Updated commit message to reflect change in feclearexcept and feraiseexcept from the glibc counterpart; - Fixed English spelling and typos; - Fixed code-style; - Changed subject line tag to make clear it is not just rs6000 code. Changes since v1[1]: - Fixed English spelling; - Fixed code-style; - Changed match operand predicate in feclearexcept and feraiseexcept; - Changed testcase options; - Minor changes in test code to be C90 compatible; - Other minor changes suggested by Segher; - Changed subject line tag (not sure if I tagged correctly or should include optabs: also) [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552024.html [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553297.html [3] https://gcc.gnu.org/pipermail/gcc-patches/2020-October/557109.html [4]
