Thank you Segher, will work on your suggestions. Umesh
On Tue, Dec 11, 2018, 19:23 Segher Boessenkool <seg...@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 <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 >