This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Thank you Segher, will work on your suggestions.

Umesh

On Tue, Dec 11, 2018, 19:23 Segher Boessenkool <segher@kernel.crashing.org
wrote:

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]