Re: [PATCH ver 2] rs6000, add overloaded DFP quantize support
Kewen, Peter: > on 2023/8/17 08:19, Carl Love wrote: > > GCC maintainers: > > > > Version 2, renamed the built-in instances. Changed the name of the > > overloaded built-in. Added the missing documentation for the new > > built-ins. Fixed typos. Changed name of the test. Updated the > > effective target for the test. Retested the patch on Power 10LE > > and > > Power 8 and Power 9. > > > > The following patch adds four built-ins for the decimal floating > point > > (DFP) quantize instructions on rs6000. The built-ins are for 64- > > bit > > and 128-bit DFP operands. > > > > The patch also adds a test case for the new builtins. > > > > The Patch has been tested on Power 10LE and Power 9 LE/BE. > > > > Please let me know if the patch is acceptable for > > mainline. Thanks. > > > > Carl Love > > > > > > > > -- > > [PATCH] rs6000, add overloaded DFP quantize support > > > > Add decimal floating point (DFP) quantize built-ins for both 64-bit > DFP > > and 128-DFP operands. In each case, there is an immediate version > and a > > variable version of the built-in. The RM value is a 2-bit constant > int > > which specifies the rounding mode to use. For the immediate > > versions > of > > the built-in, the TE field is a 5-bit constant that specifies the > value of > > the ideal exponent for the result. The built-in specifications > > are: > > > > __Decimal64 builtin_dfp_quantize (_Decimal64, _Decimal64, > > const int RM) > > __Decimal64 builtin_dfp_quantize (const int TE, _Decimal64, > > const int) > > __Decimal128 builtin_dfp_quantize (_Decimal128, _Decimal128, > > const int RM) > > __Decimal128 builtin_dfp_quantize (const int TE, _Decimal128, > > const int) > > Nit: Add the parameter name "RM" for all instances, otherwise the > readers > might feel confused what do the other two without RM mean. :) Yes, they all should have the parameter name RM. Fixed. > > > A testcase is added for the new built-in definitions. > > Nit: A PR marker line like: > > PR target/93448 > > > gcc/ChangeLog: > > * config/rs6000/dfp.md: New UNSPECDQUAN. > > (dfp_quan_, dfp_quan_i): New define_insn. > > * config/rs6000/rs6000-builtins.def (__builtin_dfp_quantize_64, > > __builtin_dfp_quantize_64i, __builtin_dfp_quantize_128, > > __builtin_dfp_quantize_128i): New buit-in definitions. > > * config/rs6000/rs6000-overload.def (__builtin_dfp_quantize, > > __builtin_dfpq_quantize): New overloaded definitions. > > These entries need updates with this new revision, also miss one > entry Fixed with the new names, added the documentation entry. > for documentation update. > > > gcc/testsuite/ > > * gcc.target/powerpc/builtin-dfp-quantize-runnable.c: New test > > case. > > Ditto, inconsistent name. Fixed with the new name of the file, pr93448.c. > > > --- > > gcc/config/rs6000/dfp.md | 25 ++- > > gcc/config/rs6000/rs6000-builtins.def | 15 ++ > > gcc/config/rs6000/rs6000-overload.def | 10 + > > gcc/doc/extend.texi | 15 ++ > > .../gcc.target/powerpc/pr93448-dfp-quantize.c | 199 > ++ > > 5 files changed, 263 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr93448-dfp- > quantize.c > > diff --git a/gcc/config/rs6000/dfp.md b/gcc/config/rs6000/dfp.md > > index 5ed8a73ac51..abd21c5db75 100644 > > --- a/gcc/config/rs6000/dfp.md > > +++ b/gcc/config/rs6000/dfp.md > > @@ -271,7 +271,8 @@ > > UNSPEC_DIEX > > UNSPEC_DSCLI > > UNSPEC_DTSTSFI > > - UNSPEC_DSCRI]) > > + UNSPEC_DSCRI > > + UNSPEC_DQUAN]) > > > > (define_code_iterator DFP_TEST [eq lt gt unordered]) > > > > @@ -395,3 +396,25 @@ > >"dscri %0,%1,%2" > >[(set_attr "type" "dfp") > > (set_attr "size" "")]) > > + > > +(define_insn "dfp_dquan_" > > I guess I mentioned this previously, I prefer "dfp_dqua_" > which aligns with the most others ... Yes, I missed that I had the extra "n" and didn't fix that part of the name. Sorry about that. Updated both define_insn definitions. > > > + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") > > +(unspec:DDTD [(match_operand:DDTD 1 "gpc_reg_operand" "d") > > + (match_operand:DDTD 2 "gpc_reg_operand" "d") > > + (match_operand:QI 3 "immediate_operand" "i")] > > + UNSPEC_DQUAN))] > > + "TARGET_DFP" > > + "dqua %0,%1,%2,%3" > > + [(set_attr "type" "dfp") > > + (set_attr "size" "")]) > > + > > +(define_insn "dfp_dquan_i" > > ..., also prefer "dfp_dquai_" here. Ditto on the name change fix. > > Please also incorporate Peter's insightful comments on predicates > and constraints on this part. OK, changed to the stricter predicate constraints. > > > + [(set
Re: [PATCH ver 2] rs6000, add overloaded DFP quantize support
Hi Carl, on 2023/8/17 08:19, Carl Love wrote: > > GCC maintainers: > > Version 2, renamed the built-in instances. Changed the name of the > overloaded built-in. Added the missing documentation for the new > built-ins. Fixed typos. Changed name of the test. Updated the > effective target for the test. Retested the patch on Power 10LE and > Power 8 and Power 9. > > The following patch adds four built-ins for the decimal floating point > (DFP) quantize instructions on rs6000. The built-ins are for 64-bit > and 128-bit DFP operands. > > The patch also adds a test case for the new builtins. > > The Patch has been tested on Power 10LE and Power 9 LE/BE. > > Please let me know if the patch is acceptable for mainline. Thanks. > > Carl Love > > > > -- > [PATCH] rs6000, add overloaded DFP quantize support > > Add decimal floating point (DFP) quantize built-ins for both 64-bit DFP > and 128-DFP operands. In each case, there is an immediate version and a > variable version of the built-in. The RM value is a 2-bit constant int > which specifies the rounding mode to use. For the immediate versions of > the built-in, the TE field is a 5-bit constant that specifies the value of > the ideal exponent for the result. The built-in specifications are: > > __Decimal64 builtin_dfp_quantize (_Decimal64, _Decimal64, > const int RM) > __Decimal64 builtin_dfp_quantize (const int TE, _Decimal64, > const int) > __Decimal128 builtin_dfp_quantize (_Decimal128, _Decimal128, >const int RM) > __Decimal128 builtin_dfp_quantize (const int TE, _Decimal128, >const int) Nit: Add the parameter name "RM" for all instances, otherwise the readers might feel confused what do the other two without RM mean. :) > > A testcase is added for the new built-in definitions. Nit: A PR marker line like: PR target/93448 > > gcc/ChangeLog: > * config/rs6000/dfp.md: New UNSPECDQUAN. > (dfp_quan_, dfp_quan_i): New define_insn. > * config/rs6000/rs6000-builtins.def (__builtin_dfp_quantize_64, > __builtin_dfp_quantize_64i, __builtin_dfp_quantize_128, > __builtin_dfp_quantize_128i): New buit-in definitions. > * config/rs6000/rs6000-overload.def (__builtin_dfp_quantize, > __builtin_dfpq_quantize): New overloaded definitions. These entries need updates with this new revision, also miss one entry for documentation update. > > gcc/testsuite/ >* gcc.target/powerpc/builtin-dfp-quantize-runnable.c: New test > case. Ditto, inconsistent name. > --- > gcc/config/rs6000/dfp.md | 25 ++- > gcc/config/rs6000/rs6000-builtins.def | 15 ++ > gcc/config/rs6000/rs6000-overload.def | 10 + > gcc/doc/extend.texi | 15 ++ > .../gcc.target/powerpc/pr93448-dfp-quantize.c | 199 ++ > 5 files changed, 263 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr93448-dfp-quantize.c > > diff --git a/gcc/config/rs6000/dfp.md b/gcc/config/rs6000/dfp.md > index 5ed8a73ac51..abd21c5db75 100644 > --- a/gcc/config/rs6000/dfp.md > +++ b/gcc/config/rs6000/dfp.md > @@ -271,7 +271,8 @@ > UNSPEC_DIEX > UNSPEC_DSCLI > UNSPEC_DTSTSFI > - UNSPEC_DSCRI]) > + UNSPEC_DSCRI > + UNSPEC_DQUAN]) > > (define_code_iterator DFP_TEST [eq lt gt unordered]) > > @@ -395,3 +396,25 @@ >"dscri %0,%1,%2" >[(set_attr "type" "dfp") > (set_attr "size" "")]) > + > +(define_insn "dfp_dquan_" I guess I mentioned this previously, I prefer "dfp_dqua_" which aligns with the most others ... > + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") > +(unspec:DDTD [(match_operand:DDTD 1 "gpc_reg_operand" "d") > + (match_operand:DDTD 2 "gpc_reg_operand" "d") > + (match_operand:QI 3 "immediate_operand" "i")] > + UNSPEC_DQUAN))] > + "TARGET_DFP" > + "dqua %0,%1,%2,%3" > + [(set_attr "type" "dfp") > + (set_attr "size" "")]) > + > +(define_insn "dfp_dquan_i" ..., also prefer "dfp_dquai_" here. Please also incorporate Peter's insightful comments on predicates and constraints on this part. > + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") > +(unspec:DDTD [(match_operand:SI 1 "const_int_operand" "n") > + (match_operand:DDTD 2 "gpc_reg_operand" "d") > + (match_operand:SI 3 "immediate_operand" "i")] > + UNSPEC_DQUAN))] > + "TARGET_DFP" > + "dquai %1,%0,%2,%3" > + [(set_attr "type" "dfp") > + (set_attr "size" "")]) > diff --git a/gcc/config/rs6000/rs6000-builtins.def > b/gcc/config/rs6000/rs6000-builtins.def > index 8a294d6c934..a7ab90771f9 100644 > --- a/gcc/config/rs6000/rs6000-builtins.def > +++ b/gcc/config/rs6000/rs6000-builtins.def > @@ -
Re: [PATCH ver 2] rs6000, add overloaded DFP quantize support
on 2023/8/17 11:11, Peter Bergner wrote: > On 8/16/23 7:19 PM, Carl Love wrote: >> +(define_insn "dfp_dquan_" >> + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") >> +(unspec:DDTD [(match_operand:DDTD 1 "gpc_reg_operand" "d") >> + (match_operand:DDTD 2 "gpc_reg_operand" "d") >> + (match_operand:QI 3 "immediate_operand" "i")] >> + UNSPEC_DQUAN))] >> + "TARGET_DFP" >> + "dqua %0,%1,%2,%3" >> + [(set_attr "type" "dfp") >> + (set_attr "size" "")]) > > operand 3 refers to the RMC operand field of the insn we are emitting. > RMC is a two bit unsigned operand, so I think the predicate should be > const_0_to_3_operand rather than immediate_operand. It's always best > to use a tighter predicate if we have one. Ditto for the other patterns > with an RMC operand. Good point! I agree it's better to use a suitable tighter predicate here, even if for now it's only used for bif expanding and the bif prototype already restricts it. > > I don't think we allow anything other than an integer for that operand > value, so I _think_ that "n" is probably a better constraint than "i"? > Ke Wen/Segher??? Yeah, I agree "n" is better for this context, it better matches your proposed const_0_to_3_operand/s5bit_cint_operand (const_int). BR, Kewen
Re: [PATCH ver 2] rs6000, add overloaded DFP quantize support
On 8/16/23 7:19 PM, Carl Love wrote: > +(define_insn "dfp_dquan_" > + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") > +(unspec:DDTD [(match_operand:DDTD 1 "gpc_reg_operand" "d") > + (match_operand:DDTD 2 "gpc_reg_operand" "d") > + (match_operand:QI 3 "immediate_operand" "i")] > + UNSPEC_DQUAN))] > + "TARGET_DFP" > + "dqua %0,%1,%2,%3" > + [(set_attr "type" "dfp") > + (set_attr "size" "")]) operand 3 refers to the RMC operand field of the insn we are emitting. RMC is a two bit unsigned operand, so I think the predicate should be const_0_to_3_operand rather than immediate_operand. It's always best to use a tighter predicate if we have one. Ditto for the other patterns with an RMC operand. I don't think we allow anything other than an integer for that operand value, so I _think_ that "n" is probably a better constraint than "i"? Ke Wen/Segher??? > +(define_insn "dfp_dquan_i" > + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") > +(unspec:DDTD [(match_operand:SI 1 "const_int_operand" "n") > + (match_operand:DDTD 2 "gpc_reg_operand" "d") > + (match_operand:SI 3 "immediate_operand" "i")] > + UNSPEC_DQUAN))] > + "TARGET_DFP" > + "dquai %1,%0,%2,%3" > + [(set_attr "type" "dfp") > + (set_attr "size" "")]) operand 1 refers to the TE operand field and that is a 5-bit signed operand. For that, I think we should be using the s5bit_cint_operand predicate, rather than const_int_operand. Peter
[PATCH ver 2] rs6000, add overloaded DFP quantize support
GCC maintainers: Version 2, renamed the built-in instances. Changed the name of the overloaded built-in. Added the missing documentation for the new built-ins. Fixed typos. Changed name of the test. Updated the effective target for the test. Retested the patch on Power 10LE and Power 8 and Power 9. The following patch adds four built-ins for the decimal floating point (DFP) quantize instructions on rs6000. The built-ins are for 64-bit and 128-bit DFP operands. The patch also adds a test case for the new builtins. The Patch has been tested on Power 10LE and Power 9 LE/BE. Please let me know if the patch is acceptable for mainline. Thanks. Carl Love -- [PATCH] rs6000, add overloaded DFP quantize support Add decimal floating point (DFP) quantize built-ins for both 64-bit DFP and 128-DFP operands. In each case, there is an immediate version and a variable version of the built-in. The RM value is a 2-bit constant int which specifies the rounding mode to use. For the immediate versions of the built-in, the TE field is a 5-bit constant that specifies the value of the ideal exponent for the result. The built-in specifications are: __Decimal64 builtin_dfp_quantize (_Decimal64, _Decimal64, const int RM) __Decimal64 builtin_dfp_quantize (const int TE, _Decimal64, const int) __Decimal128 builtin_dfp_quantize (_Decimal128, _Decimal128, const int RM) __Decimal128 builtin_dfp_quantize (const int TE, _Decimal128, const int) A testcase is added for the new built-in definitions. gcc/ChangeLog: * config/rs6000/dfp.md: New UNSPECDQUAN. (dfp_quan_, dfp_quan_i): New define_insn. * config/rs6000/rs6000-builtins.def (__builtin_dfp_quantize_64, __builtin_dfp_quantize_64i, __builtin_dfp_quantize_128, __builtin_dfp_quantize_128i): New buit-in definitions. * config/rs6000/rs6000-overload.def (__builtin_dfp_quantize, __builtin_dfpq_quantize): New overloaded definitions. gcc/testsuite/ * gcc.target/powerpc/builtin-dfp-quantize-runnable.c: New test case. --- gcc/config/rs6000/dfp.md | 25 ++- gcc/config/rs6000/rs6000-builtins.def | 15 ++ gcc/config/rs6000/rs6000-overload.def | 10 + gcc/doc/extend.texi | 15 ++ .../gcc.target/powerpc/pr93448-dfp-quantize.c | 199 ++ 5 files changed, 263 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.target/powerpc/pr93448-dfp-quantize.c diff --git a/gcc/config/rs6000/dfp.md b/gcc/config/rs6000/dfp.md index 5ed8a73ac51..abd21c5db75 100644 --- a/gcc/config/rs6000/dfp.md +++ b/gcc/config/rs6000/dfp.md @@ -271,7 +271,8 @@ UNSPEC_DIEX UNSPEC_DSCLI UNSPEC_DTSTSFI - UNSPEC_DSCRI]) + UNSPEC_DSCRI + UNSPEC_DQUAN]) (define_code_iterator DFP_TEST [eq lt gt unordered]) @@ -395,3 +396,25 @@ "dscri %0,%1,%2" [(set_attr "type" "dfp") (set_attr "size" "")]) + +(define_insn "dfp_dquan_" + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") +(unspec:DDTD [(match_operand:DDTD 1 "gpc_reg_operand" "d") + (match_operand:DDTD 2 "gpc_reg_operand" "d") + (match_operand:QI 3 "immediate_operand" "i")] + UNSPEC_DQUAN))] + "TARGET_DFP" + "dqua %0,%1,%2,%3" + [(set_attr "type" "dfp") + (set_attr "size" "")]) + +(define_insn "dfp_dquan_i" + [(set (match_operand:DDTD 0 "gpc_reg_operand" "=d") +(unspec:DDTD [(match_operand:SI 1 "const_int_operand" "n") + (match_operand:DDTD 2 "gpc_reg_operand" "d") + (match_operand:SI 3 "immediate_operand" "i")] + UNSPEC_DQUAN))] + "TARGET_DFP" + "dquai %1,%0,%2,%3" + [(set_attr "type" "dfp") + (set_attr "size" "")]) diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def index 8a294d6c934..a7ab90771f9 100644 --- a/gcc/config/rs6000/rs6000-builtins.def +++ b/gcc/config/rs6000/rs6000-builtins.def @@ -2983,6 +2983,21 @@ const unsigned long long __builtin_unpack_dec128 (_Decimal128, const int<1>); UNPACK_TD unpacktd {} + const _Decimal64 __builtin_dfp_dqua (_Decimal64, _Decimal64, \ + const int<2>); +DFPQUAN_64 dfp_dquan_dd {} + + const _Decimal64 __builtin_dfp_dquai (const int<5>, _Decimal64, \ + const int<2>); +DFPQUAN_64i dfp_dquan_idd {} + + const _Decimal128 __builtin_dfp_dquaq (_Decimal128, _Decimal128, \ +const int<2>); +DFPQUAN_128 dfp_dquan_td {} + + const _Decimal128 __builtin_dfp_dquaqi (const int<5>, _Decimal128, \ + const int<2>); +DFPQUAN_128i dfp_dquan_itd {} [crypto] cons