Hi Thank you for your suggestions. I have adjust the testcases and used this command to test . "runtest --tool gcc --tool_opts -mabi=ilp32 aarch64.exp= reload-valid-spoff.c"
The results of those tests changed from unexpected failures to unsupported. Attached please find the v5 patch. Any suggestions. Thanks, Duanbo > -----Original Message----- > From: Christophe Lyon [mailto:christophe.l...@linaro.org] > Sent: Wednesday, April 22, 2020 2:41 PM > To: duanbo (C) <duan...@huawei.com>; GCC Patches <gcc- > patc...@gcc.gnu.org>; Richard Sandiford <richard.sandif...@arm.com> > Subject: Re: [PATCH] aarch64:Add an error message in large code model for > ilp32 [PR94577] > > Hi, > > After this patch, a few tests are failing when running the testsuite with - > mabi=ilp32: > gcc.target/aarch64/pr63304_1.c (test for excess errors) > gcc.target/aarch64/pr63304_1.c scan-assembler-times adrp 6 > gcc.target/aarch64/pr70120-2.c (test for excess errors) > gcc.target/aarch64/pr94530.c (test for excess errors) > gcc.target/aarch64/reload-valid-spoff.c (test for excess errors) > > All of them fail because of the new error message: would you mind adjusting > the tests? > > Thanks > > Christophe > > On Tue, 21 Apr 2020 at 16:10, Richard Sandiford <richard.sandif...@arm.com> > wrote: > > > > "duanbo (C)" <duan...@huawei.com> writes: > > > Hi > > > > > >> -----Original Message----- > > >> From: Richard Sandiford [mailto:richard.sandif...@arm.com] > > >> Sent: Monday, April 20, 2020 10:42 PM > > >> To: duanbo (C) <duan...@huawei.com> > > >> Cc: GCC Patches <gcc-patches@gcc.gnu.org> > > >> Subject: Re: [PATCH] aarch64:Add an error message in large code > > >> model for > > >> ilp32 [PR94577] > > >> > > >> "duanbo (C)" <duan...@huawei.com> writes: > > >> > 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. > > >> > > >> My quoting made it very unclear, sorry, but here I meant we should > > >> remove the whole "default:" case. Of course, that triggers exactly > > >> the kind of bootstrap failure I mentioned: > > >> > > >> error: enumeration value ‘AARCH64_CMODEL_TINY_PIC’ not handled in > > >> switch [-Werror=switch] > > >> error: enumeration value ‘AARCH64_CMODEL_SMALL_PIC’ not handled > in > > >> switch [-Werror=switch] > > >> error: enumeration value ‘AARCH64_CMODEL_SMALL_SPIC’ not > handled in > > >> switch [-Werror=switch] > > >> > > >> I think it would be good to have: > > >> > > >> case AARCH64_CMODEL_TINY_PIC: > > >> case AARCH64_CMODEL_SMALL_PIC: > > >> case AARCH64_CMODEL_SMALL_SPIC: > > >> gcc_unreachable (); > > >> } > > >> > > >> and no "default:" case. > > >> > > >> (When I was reviewing the original patch and existing code, it > > >> wasn't obvious to me why we needed the default: case and > > >> gcc_unreachable, but that was probably just me being dumb. :-) > > >> Listing the individual case statements makes things more explicit. > > >> It also means we'll get warnings/ errors if we forget to update the > > >> switch statement for a new code model.) > > >> > > >> Can you retest and repost with that change? Sorry for the hassle. > > >> > > >> Thanks, > > >> Richard > > > > > > Sorry for misunderstanding what you mean. > > > I have made a new patch and carried a full test, no new failure witnessed. > > > Attached please find the v4 patch. > > > > Thanks, pushed to master. > > > > Richard
pr94712-v1.patch
Description: pr94712-v1.patch