| Message ID | 20240713145412.185783-1-vineetg@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] RISC-V: use fclass insns to implement isfinite, isnormal and isinf builtins | expand |
Hi Jeff, On 7/13/24 07:54, Vineet Gupta wrote: > Changes since v4: > - No functional changes. > - Use int iterator and attr to implement expanders in md > (inspired by loongarch patch. Thx Xi Ruoyao) > > Changes since v3: > - Remove '*' from define_insn for fclass > - Remove the dummy expander for fclass. > - De-duplicate the expanders code by using a helper which takes fclass > val. > > Changes since v2: > - fclass define_insn tightened to check op0 mode "X" with additional > expander w/o mode for callers. > - builtins expander explicit mode check and FAIL if mode not appropriate. > - subreg promoted handling to elide potential extension of ret val. > - Added isinf builtin with bimodal retun value as others. > > Changes since v1: > - Removed UNSPEC_{INFINITE,ISNORMAL} > - Don't hardcode SI in patterns, try to keep X to avoid potential > sign extension pitfalls. Implementation wise requires skipping > :MODE specifier in match_operand which is flagged as missing mode > warning. > --- > > Currently thsse builtins use float compare instructions which require > FP flags to be save/restore around them. > Our perf team complained this could be costly in uarch. > RV Base ISA already has FCLASS.{d,s,h} instruction to compare/identify FP > values w/o disturbing FP exception flags. > > Coincidently, upstream very recently got support for the corresponding > optabs. So this just requires wiring up in the backend. > > Tested for rv64, one additioal failure g++.dg/opt/pr107569.C needs > upstream ranger fix for the new optab. Pre-commit CI reports [1] three additional failures which are related to ranger patches in this space, same as what Loongarch folks are seeing [2] FAIL: gcc.dg/tree-ssa/range-sincos.c scan-tree-dump-not evrp "link_error" FAIL: gcc.dg/tree-ssa/vrp-float-abs-1.c scan-tree-dump-not evrp "link_error" FAIL: g++.dg/opt/pr107569.C -std=gnu++20 scan-tree-dump-times vrp1 "return 1;" 2 Here's my testing with trunk of this morning, which corroborates with the same: 2024-07-16 fec38d7987dd rtl-ssa: Fix removal of order_nodes [PR115929] rv64imafdc_zba_zbb_zbs_zicond_zfa/ lp64d/ medlow | 321 / 56 | 0 / 0 | 6 / 1 | # upstream rv64imafdc_zba_zbb_zbs_zicond_zfa/ lp64d/ medlow | 323 / 58 | 1 / 1 | 6 / 1 | # upstream + mypatch rv64imafdc_zba_zbb_zbs_zicond_zfa/ lp64d/ medlow | 321 / 56 | 0 / 0 | 6 / 1 | # upstream + mypatch + ranger patches Having said that I don't mind if you prefer the ranger patches make it in first. Thx, -Vineet [1] https://github.com/ewlu/gcc-precommit-ci/issues/1908 [2] https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656972.html > > gcc/ChangeLog: > * config/riscv/iterators.md (FCLASS_MASK): New define_int_iterator. > (fclass_optab): New define_int_attr. > * config/riscv/riscv.md: Add UNSPEC_FCLASS. > (fclass<ANYF:mode><X:mode>): New define_insn. > (<FCLASS_MASK:fclass_optab><ANYF:mode>2): New define_expand template > for isfinite, isnormal, isinf. > > gcc/testsuite/ChangeLog: > * gcc.target/riscv/fclass.c: New tests. > > Signed-off-by: Vineet Gupta <vineetg@rivosinc.com> > --- > gcc/config/riscv/iterators.md | 7 +++ > gcc/config/riscv/riscv.md | 57 +++++++++++++++++++++++++ > gcc/testsuite/gcc.target/riscv/fclass.c | 38 +++++++++++++++++ > 3 files changed, 102 insertions(+) > create mode 100644 gcc/testsuite/gcc.target/riscv/fclass.c > > diff --git a/gcc/config/riscv/iterators.md b/gcc/config/riscv/iterators.md > index 20745faa55ef..20413737f775 100644 > --- a/gcc/config/riscv/iterators.md > +++ b/gcc/config/riscv/iterators.md > @@ -219,6 +219,13 @@ > > (define_code_attr fix_uns [(fix "fix") (unsigned_fix "fixuns")]) > > +;; fclass mask values for corresponding builtins > +(define_int_iterator FCLASS_MASK [126 66 129]) > + > +(define_int_attr fclass_optab > + [(126 "isfinite") > + (66 "isnormal") > + (129 "isinf")]) > > ;; ------------------------------------------------------------------- > ;; Code Attributes > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md > index ff37125e3f28..1e1be1f5c192 100644 > --- a/gcc/config/riscv/riscv.md > +++ b/gcc/config/riscv/riscv.md > @@ -68,6 +68,7 @@ > UNSPEC_FMAX > UNSPEC_FMINM > UNSPEC_FMAXM > + UNSPEC_FCLASS > > ;; Stack tie > UNSPEC_TIE > @@ -3436,6 +3437,62 @@ > (set_attr "mode" "<UNITMODE>") > (set (attr "length") (const_int 16))]) > > +;; fclass instruction output bitmap > +;; 0 negative infinity > +;; 1 negative normal number. > +;; 2 negative subnormal number. > +;; 3 -0 > +;; 4 +0 > +;; 5 positive subnormal number. > +;; 6 positive normal number. > +;; 7 positive infinity > +;; 8 signaling NaN. > +;; 9 quiet NaN > + > +(define_insn "fclass<ANYF:mode><X:mode>" > + [(set (match_operand:X 0 "register_operand" "=r") > + (unspec [(match_operand:ANYF 1 "register_operand" " f")] > + UNSPEC_FCLASS))] > + "TARGET_HARD_FLOAT" > + "fclass.<fmt>\t%0,%1"; > + [(set_attr "type" "fcmp") > + (set_attr "mode" "<UNITMODE>")]) > + > +;; Implements optab for isfinite, isnormal, isinf > + > +(define_expand "<FCLASS_MASK:fclass_optab><ANYF:mode>2" > + [(match_operand 0 "register_operand" "=r") > + (match_operand:ANYF 1 "register_operand" " f") > + (const_int FCLASS_MASK)] > + "TARGET_HARD_FLOAT" > +{ > + if (GET_MODE (operands[0]) != SImode > + && GET_MODE (operands[0]) != word_mode) > + FAIL; > + > + rtx t = gen_reg_rtx (word_mode); > + rtx t_op0 = gen_reg_rtx (word_mode); > + > + if (TARGET_64BIT) > + emit_insn (gen_fclass<ANYF:mode>di (t, operands[1])); > + else > + emit_insn (gen_fclass<ANYF:mode>si (t, operands[1])); > + > + riscv_emit_binary (AND, t, t, GEN_INT (<FCLASS_MASK>)); > + rtx cmp = gen_rtx_NE (word_mode, t, const0_rtx); > + emit_insn (gen_cstore<mode>4 (t_op0, cmp, t, const0_rtx)); > + > + if (TARGET_64BIT) > + { > + t_op0 = gen_lowpart (SImode, t_op0); > + SUBREG_PROMOTED_VAR_P (t_op0) = 1; > + SUBREG_PROMOTED_SET (t_op0, SRP_SIGNED); > + } > + > + emit_move_insn (operands[0], t_op0); > + DONE; > +}) > + > (define_insn "*seq_zero_<X:mode><GPR:mode>" > [(set (match_operand:GPR 0 "register_operand" "=r") > (eq:GPR (match_operand:X 1 "register_operand" " r") > diff --git a/gcc/testsuite/gcc.target/riscv/fclass.c b/gcc/testsuite/gcc.target/riscv/fclass.c > new file mode 100644 > index 000000000000..ea0f173ecf4b > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/fclass.c > @@ -0,0 +1,38 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target hard_float } */ > +/* { dg-options "-march=rv64gc -mabi=lp64d -ftrapping-math" { target { rv64 } } } */ > +/* { dg-options "-march=rv32gc -mabi=ilp32d -ftrapping-math" { target { rv32 } } } */ > + > +int d_isfinite(double a) > +{ > + return __builtin_isfinite(a); > +} > + > +int d_isnormal(double a) > +{ > + return __builtin_isnormal(a); > +} > + > +int d_isinf(double a) > +{ > + return __builtin_isinf(a); > +} > + > +int f_isfinite(float a) > +{ > + return __builtin_isfinite(a); > +} > + > +int f_isnormal(float a) > +{ > + return __builtin_isnormal(a); > +} > + > +int f_isinf(float a) > +{ > + return __builtin_isinf(a); > +} > + > +/* { dg-final { scan-assembler-not {\mfrflags} } } */ > +/* { dg-final { scan-assembler-not {\mfsflags} } } */ > +/* { dg-final { scan-assembler-times {\tfclass} 6 } } */
On 7/16/24 12:26 PM, Vineet Gupta wrote: > Hi Jeff, > > On 7/13/24 07:54, Vineet Gupta wrote: >> Changes since v4: >> - No functional changes. >> - Use int iterator and attr to implement expanders in md >> (inspired by loongarch patch. Thx Xi Ruoyao) >> >> Changes since v3: >> - Remove '*' from define_insn for fclass >> - Remove the dummy expander for fclass. >> - De-duplicate the expanders code by using a helper which takes fclass >> val. >> >> Changes since v2: >> - fclass define_insn tightened to check op0 mode "X" with additional >> expander w/o mode for callers. >> - builtins expander explicit mode check and FAIL if mode not appropriate. >> - subreg promoted handling to elide potential extension of ret val. >> - Added isinf builtin with bimodal retun value as others. >> >> Changes since v1: >> - Removed UNSPEC_{INFINITE,ISNORMAL} >> - Don't hardcode SI in patterns, try to keep X to avoid potential >> sign extension pitfalls. Implementation wise requires skipping >> :MODE specifier in match_operand which is flagged as missing mode >> warning. >> --- >> >> Currently thsse builtins use float compare instructions which require >> FP flags to be save/restore around them. >> Our perf team complained this could be costly in uarch. >> RV Base ISA already has FCLASS.{d,s,h} instruction to compare/identify FP >> values w/o disturbing FP exception flags. >> >> Coincidently, upstream very recently got support for the corresponding >> optabs. So this just requires wiring up in the backend. >> >> Tested for rv64, one additioal failure g++.dg/opt/pr107569.C needs >> upstream ranger fix for the new optab. > > > Pre-commit CI reports [1] three additional failures which are related to > ranger patches in this space, same as what Loongarch folks are seeing [2] > > FAIL: gcc.dg/tree-ssa/range-sincos.c scan-tree-dump-not evrp "link_error" > FAIL: gcc.dg/tree-ssa/vrp-float-abs-1.c scan-tree-dump-not evrp "link_error" > FAIL: g++.dg/opt/pr107569.C -std=gnu++20 scan-tree-dump-times vrp1 > "return 1;" 2 > > Here's my testing with trunk of this morning, which corroborates with > the same: > 2024-07-16 fec38d7987dd rtl-ssa: Fix removal of order_nodes > [PR115929] > > rv64imafdc_zba_zbb_zbs_zicond_zfa/ lp64d/ medlow | 321 / 56 | 0 > / 0 | 6 / 1 | # upstream > rv64imafdc_zba_zbb_zbs_zicond_zfa/ lp64d/ medlow | 323 / 58 | 1 > / 1 | 6 / 1 | # upstream + mypatch > rv64imafdc_zba_zbb_zbs_zicond_zfa/ lp64d/ medlow | 321 / 56 | 0 > / 0 | 6 / 1 | # upstream + mypatch + ranger patches > > Having said that I don't mind if you prefer the ranger patches make it > in first. > > Thx, > -Vineet > > [1] https://github.com/ewlu/gcc-precommit-ci/issues/1908 > [2] https://gcc.gnu.org/pipermail/gcc-patches/2024-July/656972.html Thanks. I didn't make the connection between Hao's patch and these failures, particularly the range-sincos.c case. Not sure how I could have missed it given that test is explicitly mentioned in the first paragraph of Hao's message. I actually gave Hao a bit of feeback last week on a couple things that didn't look quite right. So hopefully we'll have that patch moving forward shortly. Let's get closure on Hao's patch, then this one can go forward. Thanks! jeff
diff --git a/gcc/config/riscv/iterators.md b/gcc/config/riscv/iterators.md index 20745faa55ef..20413737f775 100644 --- a/gcc/config/riscv/iterators.md +++ b/gcc/config/riscv/iterators.md @@ -219,6 +219,13 @@ (define_code_attr fix_uns [(fix "fix") (unsigned_fix "fixuns")]) +;; fclass mask values for corresponding builtins +(define_int_iterator FCLASS_MASK [126 66 129]) + +(define_int_attr fclass_optab + [(126 "isfinite") + (66 "isnormal") + (129 "isinf")]) ;; ------------------------------------------------------------------- ;; Code Attributes diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md index ff37125e3f28..1e1be1f5c192 100644 --- a/gcc/config/riscv/riscv.md +++ b/gcc/config/riscv/riscv.md @@ -68,6 +68,7 @@ UNSPEC_FMAX UNSPEC_FMINM UNSPEC_FMAXM + UNSPEC_FCLASS ;; Stack tie UNSPEC_TIE @@ -3436,6 +3437,62 @@ (set_attr "mode" "<UNITMODE>") (set (attr "length") (const_int 16))]) +;; fclass instruction output bitmap +;; 0 negative infinity +;; 1 negative normal number. +;; 2 negative subnormal number. +;; 3 -0 +;; 4 +0 +;; 5 positive subnormal number. +;; 6 positive normal number. +;; 7 positive infinity +;; 8 signaling NaN. +;; 9 quiet NaN + +(define_insn "fclass<ANYF:mode><X:mode>" + [(set (match_operand:X 0 "register_operand" "=r") + (unspec [(match_operand:ANYF 1 "register_operand" " f")] + UNSPEC_FCLASS))] + "TARGET_HARD_FLOAT" + "fclass.<fmt>\t%0,%1"; + [(set_attr "type" "fcmp") + (set_attr "mode" "<UNITMODE>")]) + +;; Implements optab for isfinite, isnormal, isinf + +(define_expand "<FCLASS_MASK:fclass_optab><ANYF:mode>2" + [(match_operand 0 "register_operand" "=r") + (match_operand:ANYF 1 "register_operand" " f") + (const_int FCLASS_MASK)] + "TARGET_HARD_FLOAT" +{ + if (GET_MODE (operands[0]) != SImode + && GET_MODE (operands[0]) != word_mode) + FAIL; + + rtx t = gen_reg_rtx (word_mode); + rtx t_op0 = gen_reg_rtx (word_mode); + + if (TARGET_64BIT) + emit_insn (gen_fclass<ANYF:mode>di (t, operands[1])); + else + emit_insn (gen_fclass<ANYF:mode>si (t, operands[1])); + + riscv_emit_binary (AND, t, t, GEN_INT (<FCLASS_MASK>)); + rtx cmp = gen_rtx_NE (word_mode, t, const0_rtx); + emit_insn (gen_cstore<mode>4 (t_op0, cmp, t, const0_rtx)); + + if (TARGET_64BIT) + { + t_op0 = gen_lowpart (SImode, t_op0); + SUBREG_PROMOTED_VAR_P (t_op0) = 1; + SUBREG_PROMOTED_SET (t_op0, SRP_SIGNED); + } + + emit_move_insn (operands[0], t_op0); + DONE; +}) + (define_insn "*seq_zero_<X:mode><GPR:mode>" [(set (match_operand:GPR 0 "register_operand" "=r") (eq:GPR (match_operand:X 1 "register_operand" " r") diff --git a/gcc/testsuite/gcc.target/riscv/fclass.c b/gcc/testsuite/gcc.target/riscv/fclass.c new file mode 100644 index 000000000000..ea0f173ecf4b --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/fclass.c @@ -0,0 +1,38 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target hard_float } */ +/* { dg-options "-march=rv64gc -mabi=lp64d -ftrapping-math" { target { rv64 } } } */ +/* { dg-options "-march=rv32gc -mabi=ilp32d -ftrapping-math" { target { rv32 } } } */ + +int d_isfinite(double a) +{ + return __builtin_isfinite(a); +} + +int d_isnormal(double a) +{ + return __builtin_isnormal(a); +} + +int d_isinf(double a) +{ + return __builtin_isinf(a); +} + +int f_isfinite(float a) +{ + return __builtin_isfinite(a); +} + +int f_isnormal(float a) +{ + return __builtin_isnormal(a); +} + +int f_isinf(float a) +{ + return __builtin_isinf(a); +} + +/* { dg-final { scan-assembler-not {\mfrflags} } } */ +/* { dg-final { scan-assembler-not {\mfsflags} } } */ +/* { dg-final { scan-assembler-times {\tfclass} 6 } } */
Changes since v4: - No functional changes. - Use int iterator and attr to implement expanders in md (inspired by loongarch patch. Thx Xi Ruoyao) Changes since v3: - Remove '*' from define_insn for fclass - Remove the dummy expander for fclass. - De-duplicate the expanders code by using a helper which takes fclass val. Changes since v2: - fclass define_insn tightened to check op0 mode "X" with additional expander w/o mode for callers. - builtins expander explicit mode check and FAIL if mode not appropriate. - subreg promoted handling to elide potential extension of ret val. - Added isinf builtin with bimodal retun value as others. Changes since v1: - Removed UNSPEC_{INFINITE,ISNORMAL} - Don't hardcode SI in patterns, try to keep X to avoid potential sign extension pitfalls. Implementation wise requires skipping :MODE specifier in match_operand which is flagged as missing mode warning. --- Currently thsse builtins use float compare instructions which require FP flags to be save/restore around them. Our perf team complained this could be costly in uarch. RV Base ISA already has FCLASS.{d,s,h} instruction to compare/identify FP values w/o disturbing FP exception flags. Coincidently, upstream very recently got support for the corresponding optabs. So this just requires wiring up in the backend. Tested for rv64, one additioal failure g++.dg/opt/pr107569.C needs upstream ranger fix for the new optab. gcc/ChangeLog: * config/riscv/iterators.md (FCLASS_MASK): New define_int_iterator. (fclass_optab): New define_int_attr. * config/riscv/riscv.md: Add UNSPEC_FCLASS. (fclass<ANYF:mode><X:mode>): New define_insn. (<FCLASS_MASK:fclass_optab><ANYF:mode>2): New define_expand template for isfinite, isnormal, isinf. gcc/testsuite/ChangeLog: * gcc.target/riscv/fclass.c: New tests. Signed-off-by: Vineet Gupta <vineetg@rivosinc.com> --- gcc/config/riscv/iterators.md | 7 +++ gcc/config/riscv/riscv.md | 57 +++++++++++++++++++++++++ gcc/testsuite/gcc.target/riscv/fclass.c | 38 +++++++++++++++++ 3 files changed, 102 insertions(+) create mode 100644 gcc/testsuite/gcc.target/riscv/fclass.c