| Message ID | 20190604141750.42210082@jozef-kubuntu |
|---|---|
| State | New |
| Headers | show |
| Series | Reduce code size when performing bit shifts | expand |
On 6/4/19 7:17 AM, Jozef Lawrynowicz wrote: > This patch implements 64-bit shifts in assembly code. Previously, generic C > library code from libgcc would be used to perform the shifts, which was much > more costly in terms of code size. > > I observed 700 PASS->FAIL regressions from the GCC testsuite alone when these > 64-bit shifts were implemented incorrectly, hence I've assumed there is > already adequate test coverage that shifts operate correctly, and I have not > added new tests to verify their correct execution. > > For the following program, the below code size reduction is observed: > long long a; > > int > main (void) > { > a = a >> 4; > return 0; > } > > With shift patch 3: > text data bss dec hex filename > 670 12 26 708 2c4 a.out > With new patch: > text data bss dec hex filename > 512 12 26 550 226 a.out > > Ok for trunk? > > > 0004-MSP430-Implement-64-bit-shifts-in-assembly-code.patch > > From 3b34b3d005ea63b37cf6a277395a048e55d854b2 Mon Sep 17 00:00:00 2001 > From: Jozef Lawrynowicz <jozef.l@mittosystems.com> > Date: Mon, 13 May 2019 17:55:27 +0100 > Subject: [PATCH 4/4] MSP430: Implement 64-bit shifts in assembly code > > gcc/ChangeLog > > 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> > > * config/msp430/msp430.c (msp430_expand_helper): Setup arguments which > describe how to perform MSPABI compliant 64-bit shift. > * config/msp430/msp430.md (ashldi3): New define_expand. > (ashrdi3): New define_expand. > (lshrdi3): New define_expand. > > libgcc/ChangeLog > > 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> > > * config/msp430/slli.S (__mspabi_sllll): New library function for > performing a logical left shift of a 64-bit value. > (__mspabi_srall): New library function for > performing a arithmetic right shift of a 64-bit value. > (__mspabi_srlll): New library function for > performing a logical right shift of a 64-bit value. > > gcc/testsuite/ChangeLog > > 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> > > * gcc.target/msp430/mspabi_sllll.c: New test. > * gcc.target/msp430/mspabi_srall.c: New test. > * gcc.target/msp430/mspabi_srlll.c: New test. Going to assume your assembly routines are correct :-) OK jeff
On Wed, 5 Jun 2019 16:35:14 -0600 Jeff Law <law@redhat.com> wrote: > On 6/4/19 7:17 AM, Jozef Lawrynowicz wrote: > > libgcc/ChangeLog > > > > 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> > > > > * config/msp430/slli.S (__mspabi_sllll): New library function for > > performing a logical left shift of a 64-bit value. > > (__mspabi_srall): New library function for > > performing a arithmetic right shift of a 64-bit value. > > (__mspabi_srlll): New library function for > > performing a logical right shift of a 64-bit value. > > > Going to assume your assembly routines are correct :-) > > OK > jeff I assume I implemented them correctly based on the clean regtest of GCC/G++ testsuites. But in case there might be a gap in the coverage somewhere, how about the attached new torture test to explicitly check 64-bit shifts work as expected? Passes for x86_64-linux-gnu and msp430-elf. Thanks for the reviews. Jozef
On 6/6/19 6:42 AM, Jozef Lawrynowicz wrote: > On Wed, 5 Jun 2019 16:35:14 -0600 > Jeff Law <law@redhat.com> wrote: > >> On 6/4/19 7:17 AM, Jozef Lawrynowicz wrote: >>> libgcc/ChangeLog >>> >>> 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> >>> >>> * config/msp430/slli.S (__mspabi_sllll): New library function for >>> performing a logical left shift of a 64-bit value. >>> (__mspabi_srall): New library function for >>> performing a arithmetic right shift of a 64-bit value. >>> (__mspabi_srlll): New library function for >>> performing a logical right shift of a 64-bit value. >>> >> Going to assume your assembly routines are correct :-) >> >> OK >> jeff > I assume I implemented them correctly based on the clean regtest of > GCC/G++ testsuites. But in case there might be a gap in the coverage > somewhere, how about the attached new torture test to explicitly check 64-bit > shifts work as expected? > Passes for x86_64-linux-gnu and msp430-elf. I suspect this needs to be conditional on 64bit integer support (check either at runtime with sizeof or via dejagnu effective target stuff). With that fixed this is OK. jeff
On Thu, 6 Jun 2019 11:32:49 -0600 Jeff Law <law@redhat.com> wrote: > On 6/6/19 6:42 AM, Jozef Lawrynowicz wrote: > > On Wed, 5 Jun 2019 16:35:14 -0600 > > Jeff Law <law@redhat.com> wrote: > > > >> On 6/4/19 7:17 AM, Jozef Lawrynowicz wrote: > >>> libgcc/ChangeLog > >>> > >>> 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> > >>> > >>> * config/msp430/slli.S (__mspabi_sllll): New library function for > >>> performing a logical left shift of a 64-bit value. > >>> (__mspabi_srall): New library function for > >>> performing a arithmetic right shift of a 64-bit value. > >>> (__mspabi_srlll): New library function for > >>> performing a logical right shift of a 64-bit value. > >>> > >> Going to assume your assembly routines are correct :-) > >> > >> OK > >> jeff > > I assume I implemented them correctly based on the clean regtest of > > GCC/G++ testsuites. But in case there might be a gap in the coverage > > somewhere, how about the attached new torture test to explicitly check 64-bit > > shifts work as expected? > > Passes for x86_64-linux-gnu and msp430-elf. > I suspect this needs to be conditional on 64bit integer support (check > either at runtime with sizeof or via dejagnu effective target stuff). > With that fixed this is OK. > > jeff Since it seems that the use of long long when a 64-bit type is required is much more prevalent in the testsuite that __INT64_TYPE__ (which may not be supported on all systems), I changed the types to use long long, and made the test require the longlong64 effective target keyword. Committed. Thanks, Jozef
From 3b34b3d005ea63b37cf6a277395a048e55d854b2 Mon Sep 17 00:00:00 2001 From: Jozef Lawrynowicz <jozef.l@mittosystems.com> Date: Mon, 13 May 2019 17:55:27 +0100 Subject: [PATCH 4/4] MSP430: Implement 64-bit shifts in assembly code gcc/ChangeLog 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> * config/msp430/msp430.c (msp430_expand_helper): Setup arguments which describe how to perform MSPABI compliant 64-bit shift. * config/msp430/msp430.md (ashldi3): New define_expand. (ashrdi3): New define_expand. (lshrdi3): New define_expand. libgcc/ChangeLog 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> * config/msp430/slli.S (__mspabi_sllll): New library function for performing a logical left shift of a 64-bit value. (__mspabi_srall): New library function for performing a arithmetic right shift of a 64-bit value. (__mspabi_srlll): New library function for performing a logical right shift of a 64-bit value. gcc/testsuite/ChangeLog 2019-06-04 Jozef Lawrynowicz <jozef.l@mittosystems.com> * gcc.target/msp430/mspabi_sllll.c: New test. * gcc.target/msp430/mspabi_srall.c: New test. * gcc.target/msp430/mspabi_srlll.c: New test. --- gcc/config/msp430/msp430.c | 13 +++++-- gcc/config/msp430/msp430.md | 36 +++++++++++++++++++ .../gcc.target/msp430/mspabi_sllll.c | 10 ++++++ .../gcc.target/msp430/mspabi_srall.c | 10 ++++++ .../gcc.target/msp430/mspabi_srlll.c | 10 ++++++ libgcc/config/msp430/slli.S | 33 +++++++++++++++++ libgcc/config/msp430/srai.S | 34 ++++++++++++++++++ libgcc/config/msp430/srli.S | 35 ++++++++++++++++++ 8 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.target/msp430/mspabi_sllll.c create mode 100644 gcc/testsuite/gcc.target/msp430/mspabi_srall.c create mode 100644 gcc/testsuite/gcc.target/msp430/mspabi_srlll.c diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c index 020e980b8cc..365e9eba747 100644 --- a/gcc/config/msp430/msp430.c +++ b/gcc/config/msp430/msp430.c @@ -3046,6 +3046,7 @@ msp430_expand_helper (rtx *operands, const char *helper_name, bool const_variant { rtx c, f; char *helper_const = NULL; + int arg1 = 12; int arg2 = 13; int arg1sz = 1; machine_mode arg0mode = GET_MODE (operands[0]); @@ -3079,6 +3080,13 @@ msp430_expand_helper (rtx *operands, const char *helper_name, bool const_variant arg2 = 14; arg1sz = 2; } + else if (arg1mode == DImode) + { + /* Shift value in R8:R11, shift amount in R12. */ + arg1 = 8; + arg1sz = 4; + arg2 = 12; + } if (const_variants && CONST_INT_P (operands[2]) @@ -3091,7 +3099,7 @@ msp430_expand_helper (rtx *operands, const char *helper_name, bool const_variant snprintf (helper_const, len, "%s_%d", helper_name, (int) INTVAL (operands[2])); } - emit_move_insn (gen_rtx_REG (arg1mode, 12), + emit_move_insn (gen_rtx_REG (arg1mode, arg1), operands[1]); if (!helper_const) emit_move_insn (gen_rtx_REG (arg2mode, arg2), @@ -3104,12 +3112,13 @@ msp430_expand_helper (rtx *operands, const char *helper_name, bool const_variant RTL_CONST_CALL_P (c) = 1; f = 0; - use_regs (&f, 12, arg1sz); + use_regs (&f, arg1, arg1sz); if (!helper_const) use_regs (&f, arg2, 1); add_function_usage_to (c, f); emit_move_insn (operands[0], + /* Return value will always start in R12. */ gen_rtx_REG (arg0mode, 12)); } diff --git a/gcc/config/msp430/msp430.md b/gcc/config/msp430/msp430.md index 76296a2f317..f6d688950cb 100644 --- a/gcc/config/msp430/msp430.md +++ b/gcc/config/msp430/msp430.md @@ -822,6 +822,18 @@ DONE;" ) +(define_expand "ashldi3" + [(set (match_operand:DI 0 "nonimmediate_operand") + (ashift:DI (match_operand:DI 1 "general_operand") + (match_operand:DI 2 "general_operand")))] + "" + { + /* No const_variant for 64-bit shifts. */ + msp430_expand_helper (operands, \"__mspabi_sllll\", false); + DONE; + } +) + ;;---------- ;; signed A >> C @@ -911,6 +923,18 @@ DONE;" ) +(define_expand "ashrdi3" + [(set (match_operand:DI 0 "nonimmediate_operand") + (ashift:DI (match_operand:DI 1 "general_operand") + (match_operand:DI 2 "general_operand")))] + "" + { + /* No const_variant for 64-bit shifts. */ + msp430_expand_helper (operands, \"__mspabi_srall\", false); + DONE; + } +) + ;;---------- ;; unsigned A >> C @@ -990,6 +1014,18 @@ DONE;" ) +(define_expand "lshrdi3" + [(set (match_operand:DI 0 "nonimmediate_operand") + (ashift:DI (match_operand:DI 1 "general_operand") + (match_operand:DI 2 "general_operand")))] + "" + { + /* No const_variant for 64-bit shifts. */ + msp430_expand_helper (operands, \"__mspabi_srlll\", false); + DONE; + } +) + ;;------------------------------------------------------------ ;; Function Entry/Exit diff --git a/gcc/testsuite/gcc.target/msp430/mspabi_sllll.c b/gcc/testsuite/gcc.target/msp430/mspabi_sllll.c new file mode 100644 index 00000000000..b88a8be73ff --- /dev/null +++ b/gcc/testsuite/gcc.target/msp430/mspabi_sllll.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-final { scan-assembler-not "ashldi3" } } */ +/* { dg-final { scan-assembler "__mspabi_sllll" } } */ + +long long +foo (long long a) +{ + return a << 4; +} + diff --git a/gcc/testsuite/gcc.target/msp430/mspabi_srall.c b/gcc/testsuite/gcc.target/msp430/mspabi_srall.c new file mode 100644 index 00000000000..a0aba3d43d7 --- /dev/null +++ b/gcc/testsuite/gcc.target/msp430/mspabi_srall.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-final { scan-assembler-not "ashrdi3" } } */ +/* { dg-final { scan-assembler "__mspabi_srall" } } */ + +long long +foo (long long a) +{ + return a >> 4; +} + diff --git a/gcc/testsuite/gcc.target/msp430/mspabi_srlll.c b/gcc/testsuite/gcc.target/msp430/mspabi_srlll.c new file mode 100644 index 00000000000..cb9a3744b77 --- /dev/null +++ b/gcc/testsuite/gcc.target/msp430/mspabi_srlll.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-final { scan-assembler-not "lshrdi3" } } */ +/* { dg-final { scan-assembler "__mspabi_srlll" } } */ + +unsigned long long +foo (unsigned long long a) +{ + return a >> 4; +} + diff --git a/libgcc/config/msp430/slli.S b/libgcc/config/msp430/slli.S index 89ca35a9304..9210fe6e934 100644 --- a/libgcc/config/msp430/slli.S +++ b/libgcc/config/msp430/slli.S @@ -110,3 +110,36 @@ __mspabi_slll: RET #endif +/* Logical Left Shift - R8:R11 -> R12:R15 + A 64-bit argument would normally be passed in R12:R15, but __mspabi_sllll has + special conventions, so the 64-bit value to shift is passed in R8:R11. + According to the MSPABI, the shift amount is a 64-bit value in R12:R15, but + we only use the low word in R12. */ + + .section .text.__mspabi_sllll + .global __mspabi_sllll +__mspabi_sllll: + MOV R11, R15 ; Free up R11 first + MOV R12, R11 ; Save the shift amount in R11 + MOV R10, R14 + MOV R9, R13 + MOV R8, R12 + CMP #0,R11 + JNZ 1f +#ifdef __MSP430X_LARGE__ + RETA +#else + RET +#endif +1: + RLA R12 + RLC R13 + RLC R14 + RLC R15 + ADD #-1,R11 + JNZ 1b +#ifdef __MSP430X_LARGE__ + RETA +#else + RET +#endif diff --git a/libgcc/config/msp430/srai.S b/libgcc/config/msp430/srai.S index 564f7989a8c..ed5c6a5ad7c 100644 --- a/libgcc/config/msp430/srai.S +++ b/libgcc/config/msp430/srai.S @@ -108,3 +108,37 @@ __mspabi_sral: #else RET #endif + +/* Arithmetic Right Shift - R8:R11 -> R12:R15 + A 64-bit argument would normally be passed in R12:R15, but __mspabi_srall has + special conventions, so the 64-bit value to shift is passed in R8:R11. + According to the MSPABI, the shift amount is a 64-bit value in R12:R15, but + we only use the low word in R12. */ + + .section .text.__mspabi_srall + .global __mspabi_srall +__mspabi_srall: + MOV R11, R15 ; Free up R11 first + MOV R12, R11 ; Save the shift amount in R11 + MOV R10, R14 + MOV R9, R13 + MOV R8, R12 + CMP #0, R11 + JNZ 1f +#ifdef __MSP430X_LARGE__ + RETA +#else + RET +#endif +1: + RRA R15 + RRC R14 + RRC R13 + RRC R12 + ADD #-1,R11 + JNZ 1b +#ifdef __MSP430X_LARGE__ + RETA +#else + RET +#endif diff --git a/libgcc/config/msp430/srli.S b/libgcc/config/msp430/srli.S index 4dd32ea4002..bc1b034e4b9 100644 --- a/libgcc/config/msp430/srli.S +++ b/libgcc/config/msp430/srli.S @@ -112,3 +112,38 @@ __mspabi_srll: #else RET #endif + +/* Logical Right Shift - R8:R11 -> R12:R15 + A 64-bit argument would normally be passed in R12:R15, but __mspabi_srlll has + special conventions, so the 64-bit value to shift is passed in R8:R11. + According to the MSPABI, the shift amount is a 64-bit value in R12:R15, but + we only use the low word in R12. */ + + .section .text.__mspabi_srlll + .global __mspabi_srlll +__mspabi_srlll: + MOV R11, R15 ; Free up R11 first + MOV R12, R11 ; Save the shift amount in R11 + MOV R10, R14 + MOV R9, R13 + MOV R8, R12 + CMP #0,R11 + JNZ 1f +#ifdef __MSP430X_LARGE__ + RETA +#else + RET +#endif +1: + CLRC + RRC R15 + RRC R14 + RRC R13 + RRC R12 + ADD #-1,R11 + JNZ 1b +#ifdef __MSP430X_LARGE__ + RETA +#else + RET +#endif -- 2.17.1
This patch implements 64-bit shifts in assembly code. Previously, generic C library code from libgcc would be used to perform the shifts, which was much more costly in terms of code size. I observed 700 PASS->FAIL regressions from the GCC testsuite alone when these 64-bit shifts were implemented incorrectly, hence I've assumed there is already adequate test coverage that shifts operate correctly, and I have not added new tests to verify their correct execution. For the following program, the below code size reduction is observed: long long a; int main (void) { a = a >> 4; return 0; } With shift patch 3: text data bss dec hex filename 670 12 26 708 2c4 a.out With new patch: text data bss dec hex filename 512 12 26 550 226 a.out Ok for trunk?