| Message ID | 0b7a0fc49cebb090b77ac8270cad41dd3be75758.camel@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000, 128-bit Binary Integer Operations | expand |
On Tue, 2020-08-11 at 12:22 -0700, Carl Love wrote: > Segher, Will: > > Path 3 adds support for converting to/from 128-bit integers and 128-bit > decimal floating point formats. > > Carl Love > Some cosmetic comments below. overall lgtm. Thanks, -Will > > ---------------------------------------------------------------- > Add TI to TD (128-bit DFP) and TD to TI support > > gcc/ChangeLog > > 2020-08-10 Carl Love <cel@us.ibm.com> > * config/rs6000/dfp.md (floattitd2, fixtdti2): New define_insns. > > gcc/testsuite/ChangeLog > > 2020-08-10 Carl Love <cel@us.ibm.com> > * gcc.target/powerpc/int_128bit-runnable.c: Add tests. Update test. (This test already exists). > --- > gcc/config/rs6000/dfp.md | 15 +++++ > .../gcc.target/powerpc/int_128bit-runnable.c | 64 +++++++++++++++++++ nit - Path to testcase looks strange? > 2 files changed, 79 insertions(+) > > diff --git a/gcc/config/rs6000/dfp.md b/gcc/config/rs6000/dfp.md > index 8f822732bac..ac9fe189f3e 100644 > --- a/gcc/config/rs6000/dfp.md > +++ b/gcc/config/rs6000/dfp.md > @@ -222,6 +222,13 @@ > "dcffixq %0,%1" > [(set_attr "type" "dfp")]) > > +(define_insn "floattitd2" > + [(set (match_operand:TD 0 "gpc_reg_operand" "=d") > + (float:TD (match_operand:TI 1 "gpc_reg_operand" "v")))] > + "TARGET_TI_VECTOR_OPS" > + "dcffixqq %0,%1" > + [(set_attr "type" "dfp")]) > + > ;; Convert a decimal64/128 to a decimal64/128 whose value is an integer. > ;; This is the first stage of converting it to an integer type. > Compared to some existing define_insn entries, this matches the style, looks reasonable. > @@ -241,6 +248,14 @@ > "TARGET_DFP" > "dctfix<q> %0,%1" > [(set_attr "type" "dfp")]) > + > + ;; carll Fix comment. > +(define_insn "fixtdti2" > + [(set (match_operand:TI 0 "gpc_reg_operand" "=v") > + (fix:TI (match_operand:TD 1 "gpc_reg_operand" "d")))] > + "TARGET_TI_VECTOR_OPS" > + "dctfixqq %0,%1" > + [(set_attr "type" "dfp")]) > looks reasonable. > ;; Decimal builtin support > > diff --git a/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c b/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c > index c84494fc28d..d1e69cea021 100644 > --- a/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c > +++ b/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c > @@ -38,6 +38,7 @@ > #if DEBUG > #include <stdio.h> > #include <stdlib.h> > +#include <math.h> > > > void print_i128(__int128_t val) > @@ -59,6 +60,13 @@ int main () > __int128_t arg1, result; > __uint128_t uarg2; > > + _Decimal128 arg1_dfp128, result_dfp128, expected_result_dfp128; > + > + struct conv_t { > + __uint128_t u128; > + _Decimal128 d128; > + } conv, conv2; > + > vector signed long long int vec_arg1_di, vec_arg2_di; > vector unsigned long long int vec_uarg1_di, vec_uarg2_di, vec_uarg3_di; > vector unsigned long long int vec_uresult_di; > @@ -2249,6 +2257,62 @@ int main () > abort(); > #endif > } > + > + /* DFP to __int128 and __int128 to DFP conversions */ > + /* Can't get printing of DFP values to work. Print the DFP value as an > + unsigned int so we can see the bit patterns. */ > +#if 1 I'd recommend dropping the #if 1 and matching #endif. > + conv.u128 = 0x2208000000000000ULL; > + conv.u128 = (conv.u128 << 64) | 0x4ULL; //DFP bit pattern for integer 4 > + expected_result_dfp128 = conv.d128; > + > + arg1 = 4; > + > + conv.d128 = (_Decimal128) arg1; > + > + result_dfp128 = (_Decimal128) arg1; > + if (((conv.u128 >>64) != 0x2208000000000000ULL) && > + ((conv.u128 & 0xFFFFFFFFFFFFFFFF) != 0x4ULL)) { > +#if DEBUG > + printf("ERROR: convert int128 value "); > + print_i128 (arg1); > + conv.d128 = result_dfp128; > + printf("\nto DFP value 0x%llx %llx (printed as hex bit string) ", > + (unsigned long long)((conv.u128) >>64), > + (unsigned long long)((conv.u128) & 0xFFFFFFFFFFFFFFFF)); > + > + conv.d128 = expected_result_dfp128; > + printf("\ndoes not match expected_result = 0x%llx %llx\n\n", > + (unsigned long long) (conv.u128>>64), > + (unsigned long long) (conv.u128 & 0xFFFFFFFFFFFFFFFF)); > +#else > + abort(); > +#endif > + } > +#endif > + > + expected_result = 4; > > + conv.u128 = 0x2208000000000000ULL; > + conv.u128 = (conv.u128 << 64) | 0x4ULL; // 4 as DFP > + arg1_dfp128 = conv.d128; > + > + result = (__int128_t) arg1_dfp128; > + > + if (result != expected_result) { > +#if DEBUG > + printf("ERROR: convert DFP value "); > + printf("0x%llx %llx (printed as hex bit string) ", > + (unsigned long long)(conv.u128>>64), > + (unsigned long long)(conv.u128 & 0xFFFFFFFFFFFFFFFF)); > + printf("to __int128 value = "); > + print_i128 (result); > + printf("\ndoes not match expected_result = "); > + print_i128 (expected_result); > + printf("\n"); > +#else > + abort(); > +#endif > + } > return 0; > }
Hi! On Tue, Aug 11, 2020 at 12:22:59PM -0700, Carl Love wrote: > +(define_insn "floattitd2" > + [(set (match_operand:TD 0 "gpc_reg_operand" "=d") > + (float:TD (match_operand:TI 1 "gpc_reg_operand" "v")))] > + "TARGET_TI_VECTOR_OPS" > + "dcffixqq %0,%1" > + [(set_attr "type" "dfp")]) I wonder if this should just be TARGET_POWER10 now? That goes for the whole series of course. > + ;; carll I don't think we need this comment on trunk ;-) Looks fine otherwise. Okay for trunk, modulo whatever we do with YARGET_TI_VECTOR_OPS. Thanks! Segher
Segher: On Wed, 2020-08-19 at 20:29 -0500, Segher Boessenkool wrote: > Hi! > > On Tue, Aug 11, 2020 at 12:22:59PM -0700, Carl Love wrote: > > +(define_insn "floattitd2" > > + [(set (match_operand:TD 0 "gpc_reg_operand" "=d") > > + (float:TD (match_operand:TI 1 "gpc_reg_operand" "v")))] > > + "TARGET_TI_VECTOR_OPS" > > + "dcffixqq %0,%1" > > + [(set_attr "type" "dfp")]) > > I wonder if this should just be TARGET_POWER10 now? That goes for > the > whole series of course. > <snip> > > Looks fine otherwise. Okay for trunk, modulo whatever we do with > YARGET_TI_VECTOR_OPS. Thanks! > > > Segher You commented on the TARGET_TI_VECTOR_OPS in patch 2 as well, i.e. > > + /* The -mti-vector-ops option requires ISA 3.1 support and -maltivec for > > + the 128-bit instructions. Currently, TARGET_POWER10 is sufficient to > > + enable it by default. */ > > + if (TARGET_POWER10) > > + { > > + if (rs6000_isa_flags_explicit & OPTION_MASK_VSX) > > + warning(0, ("%<-mno-altivec%> disables -mti-vector-ops (128-bit integer vector register operations).")); > > + else > > + rs6000_isa_flags |= OPTION_MASK_TI_VECTOR_OPS; > > + } > > It seems odd here that -maltivec is explicitly called out here. That > should be default on for quite a while at this point. And the actual check it for -mvsx anyway? Not sure I follow what this does at all. So this all goes way back, the following is from a discussion way back in November. We added it thinking it would be good to be able to enable/disable the 128-bit vector support, as I recall. I have to admit that I am struggling a bit to remember all the details as we discussed them back then. The following is from an old email. Hi Carl, On Mon, Nov 18, 2019 at 12:18:48PM -0800, Carl Love wrote: > Per your other note, I change the test suite check, with spelling fix, > to: > > # Return 1 if the can generate the 128-bit integer operations in ISA 3.1. > # That means the following 128-bit instructions can be generated: > # vadduqm, vsubuqm, vmsumcud, vdivsq, vdivuq, vmodsq, vmoduq, vcmpuq, vcmpsq, > # vrlq, vslq, vsrq. The -mti-vector-ops flag enables the needed support. > # The -mti-vector-ops is enabled by default for -mcpu=future. Sufficient > # to test if the vadduqm instruction is generated for ISA 3.1 support. > proc check_effective_target_ppc_native_128bit { } { > return [check_no_messages_and_pattern_nocache \ > ppc_native_128bit {\mvadduqm\M} assembly { > __int128_t test_add (__int128_t a, __int128_t b) > { return a + b; } > } {-mti-vector-ops} ] > } Ah, very good, this will work fine :-) But without that -mti-vector-ops option? > > (We also need to test what happens with -mno-altivec, etc. -- > > shouldn't > > be hard, should just do the same thing as it does on older CPUs). > > So I tried compiling the test case with -mn-altivec and -mcpu=future > and I get a GCC crash. :-( Think of it this way: it is good if developers get ICEs. That way, users get fewer. :-) > I updated the setting for rs6000_isa_flags in rs6000.c to: > > /* The -mti-vector-ops option requires ISA 3.1 support and -maltivec for > the 128-bit instructions. Currently, TARGET_FUTURE is sufficient to > enable it by default. */ > if (TARGET_FUTURE) > { > if (rs6000_isa_flags_explicit & OPTION_MASK_VSX) > warning(0, ("%<-mno-altivec%> disables -mti-vector-ops (128-bit integer vector register operations).")); > else > rs6000_isa_flags |= OPTION_MASK_TI_VECTOR_OPS; > } > > Now, I get: > > $GCC_INSTALL/bin/gcc -g -mcpu=future -mno-altivec int_128bit-runnable.c -o int_128bit-runnable > cc1: warning: ‘-mno-altivec’ disables vsx > cc1: warning: ‘-mno-altivec’ disables -mti-vector-ops (128-bit integer vector register operations). > > without -mno-altivec it compiles with no warning or errors. The object > dump has the expected vadduqm and vsubuqm instructions. That will do the trick. Maybe you want the message to be quiet, (or quieter anyway), if you get testsuite fallout? You will find out. > > > +mti-vector-ops > > > +Target Report Mask(TI_VECTOR_OPS) Var(rs6000_isa_flags) > > > +Use integer 128-bit instructions for a future architecture. > > > > For upstream we should make this an internal option (so > > Undocumented), but > > it may be handy to keep the option more visible for now, sure. > > I was kinda thinking it would probably be made internal down the road. > > So with these changes, I think we have a good base patch to build on. Yes! Thank you, this looks like it will be much less problematic than I feared for :-) Segher So, do we want to drop the option OPTION_MASK_TI_VECTOR_OPS at this point and go with just TARGET_POWER10? Carl
(Long ago...) On Wed, Aug 26, 2020 at 11:23:45AM -0700, Carl Love wrote: (Lots of context, thanks!) > So, do we want to drop the option OPTION_MASK_TI_VECTOR_OPS at this > point and go with just TARGET_POWER10? It has no value anymore now, as far as I can see? So deleting it would be good, yes. Segher
diff --git a/gcc/config/rs6000/dfp.md b/gcc/config/rs6000/dfp.md index 8f822732bac..ac9fe189f3e 100644 --- a/gcc/config/rs6000/dfp.md +++ b/gcc/config/rs6000/dfp.md @@ -222,6 +222,13 @@ "dcffixq %0,%1" [(set_attr "type" "dfp")]) +(define_insn "floattitd2" + [(set (match_operand:TD 0 "gpc_reg_operand" "=d") + (float:TD (match_operand:TI 1 "gpc_reg_operand" "v")))] + "TARGET_TI_VECTOR_OPS" + "dcffixqq %0,%1" + [(set_attr "type" "dfp")]) + ;; Convert a decimal64/128 to a decimal64/128 whose value is an integer. ;; This is the first stage of converting it to an integer type. @@ -241,6 +248,14 @@ "TARGET_DFP" "dctfix<q> %0,%1" [(set_attr "type" "dfp")]) + + ;; carll +(define_insn "fixtdti2" + [(set (match_operand:TI 0 "gpc_reg_operand" "=v") + (fix:TI (match_operand:TD 1 "gpc_reg_operand" "d")))] + "TARGET_TI_VECTOR_OPS" + "dctfixqq %0,%1" + [(set_attr "type" "dfp")]) ;; Decimal builtin support diff --git a/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c b/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c index c84494fc28d..d1e69cea021 100644 --- a/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c +++ b/gcc/testsuite/gcc.target/powerpc/int_128bit-runnable.c @@ -38,6 +38,7 @@ #if DEBUG #include <stdio.h> #include <stdlib.h> +#include <math.h> void print_i128(__int128_t val) @@ -59,6 +60,13 @@ int main () __int128_t arg1, result; __uint128_t uarg2; + _Decimal128 arg1_dfp128, result_dfp128, expected_result_dfp128; + + struct conv_t { + __uint128_t u128; + _Decimal128 d128; + } conv, conv2; + vector signed long long int vec_arg1_di, vec_arg2_di; vector unsigned long long int vec_uarg1_di, vec_uarg2_di, vec_uarg3_di; vector unsigned long long int vec_uresult_di; @@ -2249,6 +2257,62 @@ int main () abort(); #endif } + + /* DFP to __int128 and __int128 to DFP conversions */ + /* Can't get printing of DFP values to work. Print the DFP value as an + unsigned int so we can see the bit patterns. */ +#if 1 + conv.u128 = 0x2208000000000000ULL; + conv.u128 = (conv.u128 << 64) | 0x4ULL; //DFP bit pattern for integer 4 + expected_result_dfp128 = conv.d128; + + arg1 = 4; + + conv.d128 = (_Decimal128) arg1; + + result_dfp128 = (_Decimal128) arg1; + if (((conv.u128 >>64) != 0x2208000000000000ULL) && + ((conv.u128 & 0xFFFFFFFFFFFFFFFF) != 0x4ULL)) { +#if DEBUG + printf("ERROR: convert int128 value "); + print_i128 (arg1); + conv.d128 = result_dfp128; + printf("\nto DFP value 0x%llx %llx (printed as hex bit string) ", + (unsigned long long)((conv.u128) >>64), + (unsigned long long)((conv.u128) & 0xFFFFFFFFFFFFFFFF)); + + conv.d128 = expected_result_dfp128; + printf("\ndoes not match expected_result = 0x%llx %llx\n\n", + (unsigned long long) (conv.u128>>64), + (unsigned long long) (conv.u128 & 0xFFFFFFFFFFFFFFFF)); +#else + abort(); +#endif + } +#endif + + expected_result = 4; + conv.u128 = 0x2208000000000000ULL; + conv.u128 = (conv.u128 << 64) | 0x4ULL; // 4 as DFP + arg1_dfp128 = conv.d128; + + result = (__int128_t) arg1_dfp128; + + if (result != expected_result) { +#if DEBUG + printf("ERROR: convert DFP value "); + printf("0x%llx %llx (printed as hex bit string) ", + (unsigned long long)(conv.u128>>64), + (unsigned long long)(conv.u128 & 0xFFFFFFFFFFFFFFFF)); + printf("to __int128 value = "); + print_i128 (result); + printf("\ndoes not match expected_result = "); + print_i128 (expected_result); + printf("\n"); +#else + abort(); +#endif + } return 0; }
Segher, Will: Path 3 adds support for converting to/from 128-bit integers and 128-bit decimal floating point formats. Carl Love ---------------------------------------------------------------- Add TI to TD (128-bit DFP) and TD to TI support gcc/ChangeLog 2020-08-10 Carl Love <cel@us.ibm.com> * config/rs6000/dfp.md (floattitd2, fixtdti2): New define_insns. gcc/testsuite/ChangeLog 2020-08-10 Carl Love <cel@us.ibm.com> * gcc.target/powerpc/int_128bit-runnable.c: Add tests. --- gcc/config/rs6000/dfp.md | 15 +++++ .../gcc.target/powerpc/int_128bit-runnable.c | 64 +++++++++++++++++++ 2 files changed, 79 insertions(+)