Re: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84762

[Segher Boessenkool <segher at kernel dot crashing dot org>]

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  <lokeshjanghel91@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  <lokeshjanghel91@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