Hi Umesh,
On Tue, Dec 11, 2018 at 05:30:48PM +0530, Umesh Kalappa wrote:
> Please find the attached patch for the subjected issue .
>
> Do please let me know your thoughts and comments on the same .
First of all: do you have a copyright assignment with the FSF?
Second: please don't send application/octet-stream attachments.
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index ee5f183..d1c0edb 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,14 @@
+2018-12-06 Lokesh Janghel <lokeshjanghe...@gmail.com>
+
+ PR target/84762
+ * config/rs6000/rs6000.c (rs6000_return_in_msb): Retrun in svr4 for
+ small struct value.
+ (rs6000_option_override_internal): Modify the condition for aix or
+ svr4.
+ * config/rs6000/rs6000.opt : Modify the -msvr4-struct-return option.
+ * config/rs6000/rs6000-opts.h : Add enum for svr4 option (Big endian
+ and Little endian).
The changelog should not be part of the patch, but written before it.
Not as diff, just as the text it is.
Indent is one tab. Not a tab and a space, not nine spaces.
There shouldn't be trailing spaces.
There should not be a space before a colon.
"Modify XYZ." means that you should have "(XYZ): Modify." instead; but
you probably can say more than just "Modify", too. Like, _what_ have you
changed about it :-)
s/retrun/return/
The changelog should mention everything you change. I haven't checked
if it does here, but all the testcases are missing (those have their own
changelog, in gcc/testsuite/ChangeLog).
+/* Return small structs in register,
+ gnu: LSB-aligned,
+ standard: MSB-aligned */
This should end with dot space space */
+enum rs6000_svr4_struct_return {
+ SVR4_STRUCT_RETURN_GNU=1,
+ SVR4_STRUCT_RETURN_STD
+};
I think a simple boolean would be easier?
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 2765263..4751b61 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4632,7 +4632,8 @@ rs6000_option_override_internal (bool global_init_p)
/* Set aix_struct_return last, after the ABI is determined.
If -maix-struct-return or -msvr4-struct-return was explicitly
used, don't override with the ABI default. */
- if (!global_options_set.x_aix_struct_return)
+ if (!global_options_set.x_aix_struct_return
+ && !rs6000_current_svr4_struct_return)
aix_struct_return = (DEFAULT_ABI != ABI_V4 || DRAFT_V4_STRUCT_RET);
Why this change?
static bool
rs6000_return_in_msb (const_tree valtype)
{
- return (DEFAULT_ABI == ABI_ELFv2
- && BYTES_BIG_ENDIAN
+ return ((DEFAULT_ABI == ABI_ELFv2
+ || (DEFAULT_ABI == ABI_V4
+ && rs6000_current_svr4_struct_return == SVR4_STRUCT_RETURN_STD))
+ && BYTES_BIG_ENDIAN
&& AGGREGATE_TYPE_P (valtype)
&& (rs6000_function_arg_padding (TYPE_MODE (valtype), valtype)
== PAD_UPWARD));
Indents are with tabs, not eight spaces. There never should be tabs
after spaces though.
Please write this as
if (DEFAULT_ABI == ABI_ELFv2
&& BYTES_BIG_ENDIAN
&& AGGREGATE_TYPE_P (valtype)
&& (rs6000_function_arg_padding (TYPE_MODE (valtype), valtype)
== PAD_UPWARD))
return true;
if (DEFAULT_ABI == ABI_V4
&& rs6000_current_svr4_struct_return == SVR4_STRUCT_RETURN_STD
&& BYTES_BIG_ENDIAN
&& AGGREGATE_TYPE_P (valtype)
&& (rs6000_function_arg_padding (TYPE_MODE (valtype), valtype)
== PAD_UPWARD))
return true;
return false;
But, on the other hand, you should do this in rs6000_function_arg_padding
instead I think.
-msvr4-struct-return
-Target Report RejectNegative Var(aix_struct_return,0) Save
-Return small structures in registers (SVR4 default).
+msvr4-struct-return=
+Target RejectNegative Joined Enum(rs6000_svr4_struct_return)
Var(rs6000_current_svr4_struct_return)
+-msvr4-struct-return=[standard,gnu] Return small structures in registers (SVR4
default).
+
+Enum
+Name(rs6000_svr4_struct_return) Type(enum rs6000_svr4_struct_return)
+
+EnumValue
+Enum(rs6000_svr4_struct_return) String(standard) Value(SVR4_STRUCT_RETURN_STD)
+
+EnumValue
+Enum(rs6000_svr4_struct_return) String(gnu) Value(SVR4_STRUCT_RETURN_GNU)
You are removing the -msvr4-struct-return option (without =). We shouldn't
delete command line options.
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,10 @@
+2018-12-06 Lokesh Janghel <lokeshjanghe...@gmail.com>
+
+ PR target/84762
+ * gcc.target/pr84762-1.c: New testcase.
+ * gcc.target/pr84762-2.c: New testcase.
+ * gcc.target/pr84762-3.c: New testcase.
These should be before the patch as well.
+++ b/gcc/testsuite/gcc.target/powerpc/pr84762-1.c
@@ -0,0 +1,9 @@
+/* { dg-do run { target powerpc*-*-* rs6000-*-* } } */
You don't have to mention the target in gcc.target/powerpc; just say
+/* { dg-do run } */
+struct smallstruct { char a; char b; char c; };
+
+struct smallstruct f(void)
+{
+ struct smallstruct s = { 0x11, 0x22, 0x33 };
+
+ return s;
+}
And this cannot run anyway, it isn't a full program. How was this tested?
+++ b/gcc/testsuite/gcc.target/powerpc/pr84762-2.c
+/* { dg-final { scan-assembler-times "lis \[\n\]*,0x\[\n\]*" 1 } } */
+/* { dg-final { scan-assembler-times "ori \[\n\]*,\[\n\]*,0x\[\n\]*" 1 } } */
These regexps can never match; they match invalid assembler code only.
Any scan-assembler regexp ending in a star is suspect btw (the star and
the atom before it can always be removed and it will still mean the same
thing).
Segher
