[PATCH] RISC-V: Add -malign-data= option.

[PATCH] RISC-V: Add -malign-data= option.

[Ilia Diachkov]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

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=.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-RISC-V-Add-malign-data-option.patch --]
[-- Type: text/x-diff; name=0001-RISC-V-Add-malign-data-option.patch, Size: 4706 bytes --]

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

Re: [PATCH] RISC-V: Add -malign-data= option.

[Andrew Pinski]

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=.

Re: [PATCH] RISC-V: Add -malign-data= option.

[Palmer Dabbelt]

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=.

Re: [PATCH] RISC-V: Add -malign-data= option.

[Ilia Diachkov]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

>> 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=.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-RISC-V-Add-malign-data-option.patch --]
[-- Type: text/x-diff; name=0001-RISC-V-Add-malign-data-option.patch, Size: 5186 bytes --]


---
 gcc/config/riscv/riscv-opts.h |  5 +++++
 gcc/config/riscv/riscv.c      |  3 ++-
 gcc/config/riscv/riscv.h      | 17 +++++++++++------
 gcc/config/riscv/riscv.opt    | 14 ++++++++++++++
 gcc/doc/invoke.texi           | 10 +++++++++-
 5 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h
index f3031f2..d00fbe2 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_xlen,
+  riscv_align_data_type_natural
+};
+
 #endif /* ! GCC_RISCV_OPTS_H */
diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
index d61455f..bc457803 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_xlen))
     return MAX (align, BITS_PER_WORD);
   return align;
 }
diff --git a/gcc/config/riscv/riscv.h b/gcc/config/riscv/riscv.h
index 8856cee..2e27e83 100644
--- a/gcc/config/riscv/riscv.h
+++ b/gcc/config/riscv/riscv.h
@@ -168,6 +168,13 @@ along with GCC; see the file COPYING3.  If not see
    mode that should actually be used.  We allow pairs of registers.  */
 #define MAX_FIXED_MODE_SIZE GET_MODE_BITSIZE (TARGET_64BIT ? TImode : DImode)
 
+/* DATA_ALIGNMENT and LOCAL_ALIGNMENT common definition.  */
+#define RISCV_EXPAND_ALIGNMENT(COND, TYPE, ALIGN)			\
+  (((COND) && ((ALIGN) < BITS_PER_WORD)					\
+    && (TREE_CODE (TYPE) == ARRAY_TYPE					\
+	|| TREE_CODE (TYPE) == UNION_TYPE				\
+	|| TREE_CODE (TYPE) == RECORD_TYPE)) ? BITS_PER_WORD : (ALIGN))
+
 /* If defined, a C expression to compute the alignment for a static
    variable.  TYPE is the data type, and ALIGN is the alignment that
    the object would ordinarily have.  The value of this macro is used
@@ -180,18 +187,16 @@ along with GCC; see the file COPYING3.  If not see
    cause character arrays to be word-aligned so that `strcpy' calls
    that copy constants to character arrays can be done inline.  */
 
-#define DATA_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 DATA_ALIGNMENT(TYPE, ALIGN)						\
+  RISCV_EXPAND_ALIGNMENT (riscv_align_data_type == riscv_align_data_type_xlen,	\
+			  TYPE, ALIGN)
 
 /* We need this for the same reason as DATA_ALIGNMENT, namely to cause
    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)
+  RISCV_EXPAND_ALIGNMENT (true, TYPE, 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..7f0c35e 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_xlen)
+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(xlen) Value(riscv_align_data_type_xlen)
+
+EnumValue
+Enum(riscv_align_data) String(natural) Value(riscv_align_data_type_natural)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 03d2895..490cc0e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -1065,7 +1065,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
@@ -23980,6 +23981,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{xlen} uses x register width
+as an alignment value, @samp{natural} uses data natural alignment.  @samp{xlen}
+is the default.
 @end table
 
 @node RL78 Options
-- 
1.8.3.1