Hi > -----Original Message----- > From: Richard Sandiford [mailto:richard.sandif...@arm.com] > Sent: Friday, April 17, 2020 9:38 PM > To: duanbo (C) <duan...@huawei.com> > Cc: Wilco Dijkstra <wilco.dijks...@arm.com>; gcc-patches@gcc.gnu.org > Subject: Re: [PATCH PR94577] [AArch64] ：Add an error message in large > code model for ilp32 > > "duanbo (C)" <duan...@huawei.com> writes: > > Thank you for your suggestions. > > I have modified accordingly and a full test has been carried, no new failure > witnessed. > > Attached please find the new patch which has been adjusted to be suitable > for git am. > > Does the v2 patch look better ? > > > > Thanks, > > Duan bo > > > > -----Original Message----- > > From: Wilco Dijkstra [mailto:wilco.dijks...@arm.com] > > Sent: Tuesday, April 14, 2020 4:40 AM > > To: GCC Patches <gcc-patches@gcc.gnu.org>; duanbo (C) > > <duan...@huawei.com> > > Subject: Re: [PATCH PR00002] aarch64：Add an error message in large > > code model for ilp32 > > > > Hi Duanbo, > > > >> This is a simple fix for pr94577. > >> The option -mabi=ilp32 should not be used in large code model. Like x86, > using -mx32 and -mcmodel=large together will result in an error message. > >> On aarch64, there is no error message for this option conflict. > >> A solution to this problem can be found in the attached patch. > >> Bootstrap and tested on aarch64 Linux platform. No new regression > witnessed. > >> Any suggestion? > > > > Thanks for your patch, more than 4GB doesn't make any sense with ILP32 > indeed. > > A few suggestions: > > > > It would be good to also update the documentation for -mcmodel=large to > state it is incompatible with -fpic, -fPIC and -mabi=ilp32. > > > > The patch adds a another switch statement on mcmodel that ignores the > previous processing done (which may changes the selected mcmodel). It > would be safer and more concise to use one switch at the top level and in > each case use an if statement to handle the special cases. > > > > A few minor nitpics: > > > > + PR target/94577 > > + * gcc.target/aarch64/pr94577.c : New test > > > > Just like comments, there should be a '.' at the end of changelog entries. > > > > AFAICT the format isn't exactly specified, but the email header should be > like: > > > > [PATCH][AArch64] PR94577: Add an error message in large code model for > > ilp32 > > > > Sometimes the PR number is also placed in brackets. > > > > Cheers, > > Wilco > > > > > > From feb16a5e5d35d4f632e1be10ce0ac4f4c3505d22 Mon Sep 17 00:00:00 > 2001 > > From: Duan bo <duan...@huawei.com> > > Date: Wed, 15 Apr 2020 05:19:31 -0400 > > Subject: [PATCH] aarch64: Add an error message in large code model for > > ilp32 [PR94577] > > > > The option -mabi=ilp32 should not be used in large code model. An > > error message is added for the option conflict. > > > > 2020-04-15 Duan bo <duan...@huawei.com> > > > > PR target/94577 > > * config/aarch64/aarch64.c: Add an error message for option conflict. > > * doc/invoke.texi (-mcmodel=large): Mention that -mcmodel=large > is > > incompatible with -fpic, -fPIC and -mabi=ilp32. > > > > 2020-04-15 Duan bo <duan...@huawei.com> > > > > PR target/94577 > > * gcc.target/aarch64/pr94577.c: New test. > > --- > > gcc/ChangeLog | 7 ++++ > > gcc/config/aarch64/aarch64.c | 41 ++++++++++++---------- > > gcc/doc/invoke.texi | 4 ++- > > gcc/testsuite/ChangeLog | 5 +++ > > gcc/testsuite/gcc.target/aarch64/pr94577.c | 10 ++++++ > > 5 files changed, 47 insertions(+), 20 deletions(-) create mode > > 100644 gcc/testsuite/gcc.target/aarch64/pr94577.c > > > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog index > > 3c6a45e8fe7..c2f1fcb7bff 100644 > > --- a/gcc/ChangeLog > > +++ b/gcc/ChangeLog > > @@ -1,3 +1,10 @@ > > +2020-04-15 Duan bo <duan...@huawei.com> > > + > > + PR target/94577 > > + * config/aarch64/aarch64.c: Add an error message for option conflict. > > + * doc/invoke.texi (-mcmodel=large): Mention that -mcmodel=large > is > > + incompatible with -fpic, -fPIC and -mabi=ilp32. > > + > > 2020-04-14 Max Filippov <jcmvb...@gmail.com> > > > > PR target/94584 > > diff --git a/gcc/config/aarch64/aarch64.c > > b/gcc/config/aarch64/aarch64.c index 4af562a81ea..f8a38bd899a 100644 > > --- a/gcc/config/aarch64/aarch64.c > > +++ b/gcc/config/aarch64/aarch64.c > > @@ -14707,32 +14707,35 @@ aarch64_init_expanders (void) static void > > initialize_aarch64_code_model (struct gcc_options *opts) { > > - if (opts->x_flag_pic) > > + aarch64_cmodel = opts->x_aarch64_cmodel_var; > > + switch (opts->x_aarch64_cmodel_var) > > { > > - switch (opts->x_aarch64_cmodel_var) > > - { > > - case AARCH64_CMODEL_TINY: > > + case AARCH64_CMODEL_TINY: > > + if (opts->x_flag_pic) > > Minor formatting nit, but: the case statement should be indented by the > same amount as the "{" for the switch statement. The code after the case > statement should be indented by two further columns. > (The formatting is wrong in the existing code too, which is probably what > confused things.) > > > aarch64_cmodel = AARCH64_CMODEL_TINY_PIC; > > - break; > > - case AARCH64_CMODEL_SMALL: > > + break; > > + case AARCH64_CMODEL_SMALL: > > + if (opts->x_flag_pic) > > + { > > #ifdef HAVE_AS_SMALL_PIC_RELOCS > > - aarch64_cmodel = (flag_pic == 2 > > - ? AARCH64_CMODEL_SMALL_PIC > > - : AARCH64_CMODEL_SMALL_SPIC); > > + aarch64_cmodel = (flag_pic == 2 > > + ? AARCH64_CMODEL_SMALL_PIC > > + : AARCH64_CMODEL_SMALL_SPIC); > > #else > > - aarch64_cmodel = AARCH64_CMODEL_SMALL_PIC; > > + aarch64_cmodel = AARCH64_CMODEL_SMALL_PIC; > > #endif > > - break; > > - case AARCH64_CMODEL_LARGE: > > + } > > + break; > > + case AARCH64_CMODEL_LARGE: > > + if (opts->x_flag_pic) > > sorry ("code model %qs with %<-f%s%>", "large", > > opts->x_flag_pic > 1 ? "PIC" : "pic"); > > - break; > > - default: > > - gcc_unreachable (); > > - } > > - } > > - else > > - aarch64_cmodel = opts->x_aarch64_cmodel_var; > > + if (opts->x_aarch64_abi == AARCH64_ABI_ILP32) > > + sorry ("code model large not supported in ilp32 mode"); > > I think "large" should be quoted here, like it is in the pic/PIC message: > > sorry ("code model %<large%> not supported in ilp32 mode"); > > or: > > sorry ("code model %qs not supported in ilp32 mode", "large"); > > The second's probably better. Each message format string creates more > work for translators, and with the second form, there's more chance that the > format can be reused elsewhere. > > > + break; > > + default: > > + gcc_unreachable (); > > This is pre-existing, but in cases like this, it's probably better to leave > out the > default case. That way bootstrap will fail if a new code model is added. > > LGTM otherwise. > > Thanks, > Richard
Thanks for your suggestions. I have modified accordingly. Attached please find the v3 patch. Can you sponsor it if it's OK to go? Thanks, Duan bo
pr94577-v3.patch
Description: pr94577-v3.patch
