| Message ID | 929f7cec35d13fac81bdaaccb7990a23@optimitech.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Add -malign-data= option. | expand |
On Tue, Jun 25, 2019 at 3:46 PM Ilia Diachkov <ilia.diachkov@optimitech.com> wrote: > > Hello, > > This patch adds new machine specific option -malign-data={word,abi} to > RISC-V port. The option switches alignment of global variables and > constants of array/record/union types. The default value > (-malign-data=word) keeps existing way of alignment computation. Another > option value (-malign-data=abi) makes data natural alignment. It avoids > extra spaces between data to reduce code size. The measured code size > reduction is about 0.4% at -Os on EEMBC automotive 1.1 tests and > SPEC2006 C/C++ benchmarks. The patch was tested in riscv-gnu-toolchain > by dejagnu. > > Please check the patch into the trunk. Hmm, may I suggest use "natural" rather than "abi" and 32bit or 64bit rather than "word"; it is not obvious what abi means and it is not obvious what word means here; it could be either 32bit or 64bit depending on the option. Also my other suggestion is create a new macro where you pass riscv_align_data_type == riscv_align_data_type_word for the "(ALIGN) < BITS_PER_WORD) " check to reduce the code duplication. Thanks, Andrew Pinski > > Best regards, > Ilia. > > gcc/ > * config/riscv/riscv-opts.h (struct riscv_align_data): Added. > * config/riscv/riscv.c (riscv_constant_alignment): Use > riscv_align_data_type. > * config/riscv/riscv.h (DATA_ALIGNMENT): Use riscv_align_data_type. > (LOCAL_ALIGNMENT): Set to old DATA_ALIGNMENT value. > * config/riscv/riscv.opt (malign-data): New. > * doc/invoke.texi (RISC-V Options): Document -malign-data=.
On Tue, 25 Jun 2019 15:58:53 PDT (-0700), pinskia@gmail.com wrote: > On Tue, Jun 25, 2019 at 3:46 PM Ilia Diachkov > <ilia.diachkov@optimitech.com> wrote: >> >> Hello, >> >> This patch adds new machine specific option -malign-data={word,abi} to >> RISC-V port. The option switches alignment of global variables and >> constants of array/record/union types. The default value >> (-malign-data=word) keeps existing way of alignment computation. Another >> option value (-malign-data=abi) makes data natural alignment. It avoids >> extra spaces between data to reduce code size. The measured code size >> reduction is about 0.4% at -Os on EEMBC automotive 1.1 tests and >> SPEC2006 C/C++ benchmarks. The patch was tested in riscv-gnu-toolchain >> by dejagnu. >> >> Please check the patch into the trunk. > > Hmm, may I suggest use "natural" rather than "abi" and 32bit or 64bit > rather than "word"; it is not obvious what abi means and it is not > obvious what word means here; it could be either 32bit or 64bit > depending on the option. It's actually worse: in RISC-V "word" always means 32-bit (BITS_PER_WORD is a GCC name). "natural" seems like a good term for the "align to the size of the type". The RISC-V term for "the width of an integer register" is "xlen", so I think that's a good bet for the other option. > Also my other suggestion is create a new macro where you pass > riscv_align_data_type == riscv_align_data_type_word for the "(ALIGN) < > BITS_PER_WORD) " check to reduce the code duplication. Additionally, has this been tested with "-mstrict-align"? The generated code can be awful, but if it's not correct then we should throw an error on that combination. > > Thanks, > Andrew Pinski > >> >> Best regards, >> Ilia. >> >> gcc/ >> * config/riscv/riscv-opts.h (struct riscv_align_data): Added. >> * config/riscv/riscv.c (riscv_constant_alignment): Use >> riscv_align_data_type. >> * config/riscv/riscv.h (DATA_ALIGNMENT): Use riscv_align_data_type. >> (LOCAL_ALIGNMENT): Set to old DATA_ALIGNMENT value. >> * config/riscv/riscv.opt (malign-data): New. >> * doc/invoke.texi (RISC-V Options): Document -malign-data=.
>> Hmm, may I suggest use "natural" rather than "abi" and 32bit or 64bit >> rather than "word"; it is not obvious what abi means and it is not >> obvious what word means here; it could be either 32bit or 64bit >> depending on the option. > > It's actually worse: in RISC-V "word" always means 32-bit > (BITS_PER_WORD is a > GCC name). "natural" seems like a good term for the "align to the size > of the > type". The RISC-V term for "the width of an integer register" is > "xlen", so I > think that's a good bet for the other option. > >> Also my other suggestion is create a new macro where you pass >> riscv_align_data_type == riscv_align_data_type_word for the "(ALIGN) < >> BITS_PER_WORD) " check to reduce the code duplication. Thanks, Andrew and Palmer. I have updated the patch (see attached) by new option values "natural" and "xlen". Also I have added macro RISCV_EXPAND_ALIGNMENT similar to one implemented in ARM. > Additionally, has this been tested with "-mstrict-align"? The > generated code > can be awful, but if it's not correct then we should throw an error on > that > combination. I have tested the new option with -mstrict-align and found no influence on each other. Indeed, -malign-data=xlen with -mstrict-align works in the same way as you run the current gcc with -mstrict-align. What about -malign-data=natural with -mstrict-align, I have tested it by dejagnu with modified target_board which additionally passes -malign-data=natural (the first run) and -malign-data=natural with -mstrict-align (the second run). Both runs shown the same result. Best regards, Ilia. gcc/ * config/riscv/riscv-opts.h (struct riscv_align_data): Added. * config/riscv/riscv.c (riscv_constant_alignment): Use riscv_align_data_type. * config/riscv/riscv.h (RISCV_EXPAND_ALIGNMENT): Added. (DATA_ALIGNMENT): Use RISCV_EXPAND_ALIGNMENT. (LOCAL_ALIGNMENT): Use RISCV_EXPAND_ALIGNMENT. * config/riscv/riscv.opt (malign-data): New. * doc/invoke.texi (RISC-V Options): Document -malign-data=.
From c183fbefb9b7b53eb066cbfdaa907b6087271029 Mon Sep 17 00:00:00 2001 From: Ilia Diachkov <ilia.diachkov@optimitech.com> Date: Wed, 26 Jun 2019 01:33:20 +0300 Subject: [PATCH] RISC-V: Add -malign-data= option. --- gcc/config/riscv/riscv-opts.h | 5 +++++ gcc/config/riscv/riscv.c | 3 ++- gcc/config/riscv/riscv.h | 10 +++++++--- gcc/config/riscv/riscv.opt | 14 ++++++++++++++ gcc/doc/invoke.texi | 10 +++++++++- 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h index f3031f2..c1f7fa1 100644 --- a/gcc/config/riscv/riscv-opts.h +++ b/gcc/config/riscv/riscv-opts.h @@ -46,4 +46,9 @@ enum riscv_microarchitecture_type { }; extern enum riscv_microarchitecture_type riscv_microarchitecture; +enum riscv_align_data { + riscv_align_data_type_word, + riscv_align_data_type_abi +}; + #endif /* ! GCC_RISCV_OPTS_H */ diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c index d61455f..08418ce 100644 --- a/gcc/config/riscv/riscv.c +++ b/gcc/config/riscv/riscv.c @@ -4904,7 +4904,8 @@ riscv_can_change_mode_class (machine_mode, machine_mode, reg_class_t rclass) static HOST_WIDE_INT riscv_constant_alignment (const_tree exp, HOST_WIDE_INT align) { - if (TREE_CODE (exp) == STRING_CST || TREE_CODE (exp) == CONSTRUCTOR) + if ((TREE_CODE (exp) == STRING_CST || TREE_CODE (exp) == CONSTRUCTOR) + && (riscv_align_data_type == riscv_align_data_type_word)) return MAX (align, BITS_PER_WORD); return align; } diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h index 8856cee..bace9d9 100644 --- a/gcc/config/riscv/riscv.h +++ b/gcc/config/riscv/riscv.h @@ -181,7 +181,8 @@ along with GCC; see the file COPYING3. If not see that copy constants to character arrays can be done inline. */ #define DATA_ALIGNMENT(TYPE, ALIGN) \ - ((((ALIGN) < BITS_PER_WORD) \ + (((riscv_align_data_type == riscv_align_data_type_word) \ + && ((ALIGN) < BITS_PER_WORD) \ && (TREE_CODE (TYPE) == ARRAY_TYPE \ || TREE_CODE (TYPE) == UNION_TYPE \ || TREE_CODE (TYPE) == RECORD_TYPE)) ? BITS_PER_WORD : (ALIGN)) @@ -190,8 +191,11 @@ along with GCC; see the file COPYING3. If not see character arrays to be word-aligned so that `strcpy' calls that copy constants to character arrays can be done inline, and 'strcmp' can be optimised to use word loads. */ -#define LOCAL_ALIGNMENT(TYPE, ALIGN) \ - DATA_ALIGNMENT (TYPE, ALIGN) +#define LOCAL_ALIGNMENT(TYPE, ALIGN) \ + ((((ALIGN) < BITS_PER_WORD) \ + && (TREE_CODE (TYPE) == ARRAY_TYPE \ + || TREE_CODE (TYPE) == UNION_TYPE \ + || TREE_CODE (TYPE) == RECORD_TYPE)) ? BITS_PER_WORD : (ALIGN)) /* Define if operations between registers always perform the operation on the full register even if a narrower mode is specified. */ diff --git a/gcc/config/riscv/riscv.opt b/gcc/config/riscv/riscv.opt index 3b25f9a..a9b8ab5 100644 --- a/gcc/config/riscv/riscv.opt +++ b/gcc/config/riscv/riscv.opt @@ -131,3 +131,17 @@ Mask(RVE) mriscv-attribute Target Report Var(riscv_emit_attribute_p) Init(-1) Emit RISC-V ELF attribute. + +malign-data= +Target RejectNegative Joined Var(riscv_align_data_type) Enum(riscv_align_data) Init(riscv_align_data_type_word) +Use the given data alignment. + +Enum +Name(riscv_align_data) Type(enum riscv_align_data) +Known data alignment choices (for use with the -malign-data= option): + +EnumValue +Enum(riscv_align_data) String(word) Value(riscv_align_data_type_word) + +EnumValue +Enum(riscv_align_data) String(abi) Value(riscv_align_data_type_abi) diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 50e50e3..55c08b3 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -1059,7 +1059,8 @@ See RS/6000 and PowerPC Options. -mcmodel=medlow -mcmodel=medany @gol -mexplicit-relocs -mno-explicit-relocs @gol -mrelax -mno-relax @gol --mriscv-attribute -mmo-riscv-attribute} +-mriscv-attribute -mmo-riscv-attribute @gol +-malign-data=@var{type}} @emph{RL78 Options} @gccoptlist{-msim -mmul=none -mmul=g13 -mmul=g14 -mallregs @gol @@ -23881,6 +23882,13 @@ linker relaxations. @itemx -mno-emit-attribute Emit (do not emit) RISC-V attribute to record extra information into ELF objects. This feature requires at least binutils 2.32. + +@item -malign-data=@var{type} +@opindex malign-data +Control how GCC aligns variables and constants of array, structure or union +types. Supported values for @var{type} are @samp{word} may increase alignment +to one machine word, @samp{abi} uses alignment value as specified by the ABI. +@samp{word} is the default. @end table @node RL78 Options -- 1.8.3.1
Hello, This patch adds new machine specific option -malign-data={word,abi} to RISC-V port. The option switches alignment of global variables and constants of array/record/union types. The default value (-malign-data=word) keeps existing way of alignment computation. Another option value (-malign-data=abi) makes data natural alignment. It avoids extra spaces between data to reduce code size. The measured code size reduction is about 0.4% at -Os on EEMBC automotive 1.1 tests and SPEC2006 C/C++ benchmarks. The patch was tested in riscv-gnu-toolchain by dejagnu. Please check the patch into the trunk. Best regards, Ilia. gcc/ * config/riscv/riscv-opts.h (struct riscv_align_data): Added. * config/riscv/riscv.c (riscv_constant_alignment): Use riscv_align_data_type. * config/riscv/riscv.h (DATA_ALIGNMENT): Use riscv_align_data_type. (LOCAL_ALIGNMENT): Set to old DATA_ALIGNMENT value. * config/riscv/riscv.opt (malign-data): New. * doc/invoke.texi (RISC-V Options): Document -malign-data=.