| Message ID | 20210223101442.18962-1-yunqiang.su@cipunited.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/2] MIPS: Not trigger error for pre-R6 and -mcompact-branches=always | expand |
On 2/23/21 3:14 AM, YunQiang Su wrote: > For MIPSr6, we may wish to use compact-branches only. > Currently, we have to use `always' option, while it is mark as conflict > with pre-R6. > cc1: error: unsupported combination: ‘mips32r2’ -mcompact-branches=always > Just ignore -mcompact-branches=always for pre-R6. > > This patch also defines > __mips_compact_branches_never > __mips_compact_branches_always > __mips_compact_branches_optimal > predefined macros > > gcc/ChangeLog: > * config/mips/mips.c (mips_option_override): > * config/mips/mips.h (TARGET_RTP_PIC): not trigger error for > compact-branches=always for pre-R6. > (TARGET_CB_NEVER): Likewise. > (TARGET_CB_ALWAYS): Likewise. > (struct mips_cpu_info): define macros for compact branch policy. > * doc/invoke.texi: Document "always" with pre-R6. > > gcc/testsuite/ChangeLog: > * gcc.target/mips/compact-branches-1.c: add isa_rev>=6. > * gcc.target/mips/mips.exp: don't add -mipsXXr6 option for > -mcompact-branches=always. It is usable for pre-R6 now. > * gcc.target/mips/compact-branches-8.c: New test. > * gcc.target/mips/compact-branches-9.c: New test. So I think Maciej's comment was that you simply shouldn't be using -mcompact-branches=always at mips32r2 (or anything pre-r6) together. I think what you're trying to do here is set up a scenario where you're defaulting to mips32r6 and compact-branches, but not error if something specifies -mcpu=mips32r2 or something similar, right? jeff
On Wed, 3 Mar 2021, Jeff Law wrote: > > gcc/testsuite/ChangeLog: > > * gcc.target/mips/compact-branches-1.c: add isa_rev>=6. > > * gcc.target/mips/mips.exp: don't add -mipsXXr6 option for > > -mcompact-branches=always. It is usable for pre-R6 now. > > * gcc.target/mips/compact-branches-8.c: New test. > > * gcc.target/mips/compact-branches-9.c: New test. > So I think Maciej's comment was that you simply shouldn't be using > -mcompact-branches=always at mips32r2 (or anything pre-r6) together. > > I think what you're trying to do here is set up a scenario where you're > defaulting to mips32r6 and compact-branches, but not error if something > specifies -mcpu=mips32r2 or something similar, right? Actually what I had in mind was relaxing the strictness of the `always' case. I have replied in a more elaborate manner in the original thread where I raised my concerns. I have not looked through the code of any newer proposals. Maciej
> > On 2/23/21 3:14 AM, YunQiang Su wrote: > > For MIPSr6, we may wish to use compact-branches only. > > Currently, we have to use `always' option, while it is mark as > > conflict with pre-R6. > > cc1: error: unsupported combination: ‘mips32r2’ > > -mcompact-branches=always Just ignore -mcompact-branches=always for > pre-R6. > > > > This patch also defines > > __mips_compact_branches_never > > __mips_compact_branches_always > > __mips_compact_branches_optimal > > predefined macros > > > > gcc/ChangeLog: > > * config/mips/mips.c (mips_option_override): > > * config/mips/mips.h (TARGET_RTP_PIC): not trigger error for > > compact-branches=always for pre-R6. > > (TARGET_CB_NEVER): Likewise. > > (TARGET_CB_ALWAYS): Likewise. > > (struct mips_cpu_info): define macros for compact branch policy. > > * doc/invoke.texi: Document "always" with pre-R6. > > > > gcc/testsuite/ChangeLog: > > * gcc.target/mips/compact-branches-1.c: add isa_rev>=6. > > * gcc.target/mips/mips.exp: don't add -mipsXXr6 option for > > -mcompact-branches=always. It is usable for pre-R6 now. > > * gcc.target/mips/compact-branches-8.c: New test. > > * gcc.target/mips/compact-branches-9.c: New test. > So I think Maciej's comment was that you simply shouldn't be using > -mcompact-branches=always at mips32r2 (or anything pre-r6) together. > > I think what you're trying to do here is set up a scenario where you're > defaulting to mips32r6 and compact-branches, but not error if something > specifies -mcpu=mips32r2 or something similar, right? > Yes. If we introduce the build time option, and configure gcc with always, then gcc will always try to Pass -mconpact-branches=always to cc1, even we use something like: mipsisa32r6el-linux-gnu-gcc -mips32r2 -c xx.c It may break something. > jeff
On 3/3/2021 8:33 PM, yunqiang.su@cipunited.com wrote: >> On 2/23/21 3:14 AM, YunQiang Su wrote: >>> For MIPSr6, we may wish to use compact-branches only. >>> Currently, we have to use `always' option, while it is mark as >>> conflict with pre-R6. >>> cc1: error: unsupported combination: ‘mips32r2’ >>> -mcompact-branches=always Just ignore -mcompact-branches=always for >> pre-R6. >>> This patch also defines >>> __mips_compact_branches_never >>> __mips_compact_branches_always >>> __mips_compact_branches_optimal >>> predefined macros >>> >>> gcc/ChangeLog: >>> * config/mips/mips.c (mips_option_override): >>> * config/mips/mips.h (TARGET_RTP_PIC): not trigger error for >>> compact-branches=always for pre-R6. >>> (TARGET_CB_NEVER): Likewise. >>> (TARGET_CB_ALWAYS): Likewise. >>> (struct mips_cpu_info): define macros for compact branch policy. >>> * doc/invoke.texi: Document "always" with pre-R6. >>> >>> gcc/testsuite/ChangeLog: >>> * gcc.target/mips/compact-branches-1.c: add isa_rev>=6. >>> * gcc.target/mips/mips.exp: don't add -mipsXXr6 option for >>> -mcompact-branches=always. It is usable for pre-R6 now. >>> * gcc.target/mips/compact-branches-8.c: New test. >>> * gcc.target/mips/compact-branches-9.c: New test. >> So I think Maciej's comment was that you simply shouldn't be using >> -mcompact-branches=always at mips32r2 (or anything pre-r6) together. >> >> I think what you're trying to do here is set up a scenario where you're >> defaulting to mips32r6 and compact-branches, but not error if something >> specifies -mcpu=mips32r2 or something similar, right? >> > Yes. If we introduce the build time option, and configure gcc with always, then gcc will always try to > Pass -mconpact-branches=always to cc1, even we use something like: > mipsisa32r6el-linux-gnu-gcc -mips32r2 -c xx.c > It may break something. So would it be possible to make the mips32rX (for X <6) option also turn off compact-branches? Maciej, is that less problematical from your standpoint? Or is this just ultimately a bad idea from start to finish? Jeff
On Sat, 20 Mar 2021, Jeff Law wrote: > > > I think what you're trying to do here is set up a scenario where you're > > > defaulting to mips32r6 and compact-branches, but not error if something > > > specifies -mcpu=mips32r2 or something similar, right? > > > > > Yes. If we introduce the build time option, and configure gcc with always, > > then gcc will always try to > > Pass -mconpact-branches=always to cc1, even we use something like: > > mipsisa32r6el-linux-gnu-gcc -mips32r2 -c xx.c > > It may break something. > > So would it be possible to make the mips32rX (for X <6) option also turn off > compact-branches? Maciej, is that less problematical from your standpoint? > Or is this just ultimately a bad idea from start to finish? I don't expect anything to break if we allow `-mcompact-branches=always' below R6, whether defaulted or used explicitly. Given that currently it's a hard error, it's not a scenario that anyone could rely on. I don't know offhand if bad code that does not assemble is going to be produced in that case, but I doubt it as individual instruction patterns are routinely guarded by an ISA level check. Also we have some compact branches or jumps to choose from below R6, such as with the MIPS16e ISA or the microMIPSr3 ISA, so the option does make some sense semantically if not functionally (observe the reservation saying: "a compact branch instruction will be generated if available", so even now we reserve the right to produce a delay slot form despite the option being active, although the wording does imply best efforts). Regardless I would not require the option to fully support those ISA variations as a prerequisite for YunQiang's change. I think it would be enough if we documented that it is effective for R6+ only (by modifying the current note in the manual). If a need or desire arises, then a further update can be made in the future. I think this is GCC 12 material however, we're well into a feature freeze now and it is not a bug fix. It will give people plenty of time too to run regression testing with `-mcompact-branches=always' combined with a representative set of ISA levels. Maciej
> -----邮件原件----- > 发件人: Jeff Law <jeffreyalaw@gmail.com> > 发送时间: 2021年3月20日 23:42 > 收件人: yunqiang.su@cipunited.com; gcc-patches@gcc.gnu.org > 抄送: macro@orcam.me.uk; law@redhat.com; doko@debian.org; > syq@debian.org; jiaxun.yang@flygoat.com > 主题: Re: 回复: [PATCH v4 1/2] MIPS: Not trigger error for pre-R6 and > -mcompact-branches=always > > > On 3/3/2021 8:33 PM, yunqiang.su@cipunited.com wrote: > >> On 2/23/21 3:14 AM, YunQiang Su wrote: > >>> For MIPSr6, we may wish to use compact-branches only. > >>> Currently, we have to use `always' option, while it is mark as > >>> conflict with pre-R6. > >>> cc1: error: unsupported combination: ‘mips32r2’ > >>> -mcompact-branches=always Just ignore -mcompact-branches=always > for > >> pre-R6. > >>> This patch also defines > >>> __mips_compact_branches_never > >>> __mips_compact_branches_always > >>> __mips_compact_branches_optimal predefined macros > >>> > >>> gcc/ChangeLog: > >>> * config/mips/mips.c (mips_option_override): > >>> * config/mips/mips.h (TARGET_RTP_PIC): not trigger error for > >>> compact-branches=always for pre-R6. > >>> (TARGET_CB_NEVER): Likewise. > >>> (TARGET_CB_ALWAYS): Likewise. > >>> (struct mips_cpu_info): define macros for compact branch policy. > >>> * doc/invoke.texi: Document "always" with pre-R6. > >>> > >>> gcc/testsuite/ChangeLog: > >>> * gcc.target/mips/compact-branches-1.c: add isa_rev>=6. > >>> * gcc.target/mips/mips.exp: don't add -mipsXXr6 option for > >>> -mcompact-branches=always. It is usable for pre-R6 now. > >>> * gcc.target/mips/compact-branches-8.c: New test. > >>> * gcc.target/mips/compact-branches-9.c: New test. > >> So I think Maciej's comment was that you simply shouldn't be using > >> -mcompact-branches=always at mips32r2 (or anything pre-r6) together. > >> > >> I think what you're trying to do here is set up a scenario where > >> you're defaulting to mips32r6 and compact-branches, but not error if > >> something specifies -mcpu=mips32r2 or something similar, right? > >> > > Yes. If we introduce the build time option, and configure gcc with > > always, then gcc will always try to Pass -mconpact-branches=always to cc1, > even we use something like: > > mipsisa32r6el-linux-gnu-gcc -mips32r2 -c xx.c It may break > > something. > > So would it be possible to make the mips32rX (for X <6) option also turn off Yes. It is possible, while it may introduce lots of complexity to the code. > compact-branches? Maciej, is that less problematical from your > standpoint? Or is this just ultimately a bad idea from start to finish? > > > Jeff
diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index 8bd2d29552e..9a75dd61031 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -20107,13 +20107,7 @@ mips_option_override (void) target_flags |= MASK_ODD_SPREG; } - if (!ISA_HAS_COMPACT_BRANCHES && mips_cb == MIPS_CB_ALWAYS) - { - error ("unsupported combination: %qs%s %s", - mips_arch_info->name, TARGET_MICROMIPS ? " -mmicromips" : "", - "-mcompact-branches=always"); - } - else if (!ISA_HAS_DELAY_SLOTS && mips_cb == MIPS_CB_NEVER) + if (!ISA_HAS_DELAY_SLOTS && mips_cb == MIPS_CB_NEVER) { error ("unsupported combination: %qs%s %s", mips_arch_info->name, TARGET_MICROMIPS ? " -mmicromips" : "", diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index b4a60a55d80..b8399fe1b0d 100644 --- a/gcc/config/mips/mips.h +++ b/gcc/config/mips/mips.h @@ -103,11 +103,9 @@ struct mips_cpu_info { #define TARGET_RTP_PIC (TARGET_VXWORKS_RTP && flag_pic) /* Compact branches must not be used if the user either selects the - 'never' policy or the 'optimal' policy on a core that lacks + 'never' policy or the 'optimal' / 'always' policy on a core that lacks compact branch instructions. */ -#define TARGET_CB_NEVER (mips_cb == MIPS_CB_NEVER \ - || (mips_cb == MIPS_CB_OPTIMAL \ - && !ISA_HAS_COMPACT_BRANCHES)) +#define TARGET_CB_NEVER (mips_cb == MIPS_CB_NEVER || !ISA_HAS_COMPACT_BRANCHES) /* Compact branches may be used if the user either selects the 'always' policy or the 'optimal' policy on a core that supports @@ -117,10 +115,11 @@ struct mips_cpu_info { && ISA_HAS_COMPACT_BRANCHES)) /* Compact branches must always be generated if the user selects - the 'always' policy or the 'optimal' policy om a core that - lacks delay slot branch instructions. */ -#define TARGET_CB_ALWAYS (mips_cb == MIPS_CB_ALWAYS \ - || (mips_cb == MIPS_CB_OPTIMAL \ + the 'always' policy on a core support compact branches, + or the 'optimal' policy on a core that lacks delay slot branch instructions. */ +#define TARGET_CB_ALWAYS ((mips_cb == MIPS_CB_ALWAYS \ + && ISA_HAS_COMPACT_BRANCHES) \ + || (mips_cb == MIPS_CB_OPTIMAL \ && !ISA_HAS_DELAY_SLOTS)) /* Special handling for JRC that exists in microMIPSR3 as well as R6 @@ -655,6 +654,13 @@ struct mips_cpu_info { builtin_define ("__mips_no_lxc1_sxc1"); \ if (!ISA_HAS_UNFUSED_MADD4 && !ISA_HAS_FUSED_MADD4) \ builtin_define ("__mips_no_madd4"); \ + \ + if (TARGET_CB_NEVER) \ + builtin_define ("__mips_compact_branches_never"); \ + else if (TARGET_CB_ALWAYS) \ + builtin_define ("__mips_compact_branches_always"); \ + else \ + builtin_define ("__mips_compact_branches_optimal"); \ } \ while (0) diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index ea315f1be58..50c4556d293 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -25237,6 +25237,7 @@ instruction is not available, a delay slot form of the branch will be used instead. This option is supported from MIPS Release 6 onwards. +If it is used for pre-R6 target, it will be just ignored. The @option{-mcompact-branches=optimal} option will cause a delay slot branch to be used if one is available in the current ISA and the delay diff --git a/gcc/testsuite/gcc.target/mips/compact-branches-1.c b/gcc/testsuite/gcc.target/mips/compact-branches-1.c index 9c7365e2659..6b8e1978d87 100644 --- a/gcc/testsuite/gcc.target/mips/compact-branches-1.c +++ b/gcc/testsuite/gcc.target/mips/compact-branches-1.c @@ -1,4 +1,4 @@ -/* { dg-options "-mcompact-branches=always -mno-micromips" } */ +/* { dg-options "-mcompact-branches=always -mno-micromips isa_rev>=6" } */ int glob; void diff --git a/gcc/testsuite/gcc.target/mips/compact-branches-8.c b/gcc/testsuite/gcc.target/mips/compact-branches-8.c new file mode 100644 index 00000000000..1290cedf4b5 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/compact-branches-8.c @@ -0,0 +1,10 @@ +/* { dg-options "-mno-abicalls -mcompact-branches=always isa_rev<=5" } */ +void bar (int); + +void +foo () +{ + bar (1); +} + +/* { dg-final { scan-assembler "\t(j|jal)\t" } } */ diff --git a/gcc/testsuite/gcc.target/mips/compact-branches-9.c b/gcc/testsuite/gcc.target/mips/compact-branches-9.c new file mode 100644 index 00000000000..4b23bf456d1 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/compact-branches-9.c @@ -0,0 +1,10 @@ +/* { dg-options "-mno-abicalls -fno-PIC -mcompact-branches=always isa_rev>=6" } */ +void bar (int); + +void +foo () +{ + bar (1); +} + +/* { dg-final { scan-assembler "\t(bc|balc)\t" } } */ diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp index 01292316944..4b46b97a884 100644 --- a/gcc/testsuite/gcc.target/mips/mips.exp +++ b/gcc/testsuite/gcc.target/mips/mips.exp @@ -1163,10 +1163,8 @@ proc mips-dg-options { args } { # We need a revision 6 or better ISA for: # # - When the LSA instruction is required - # - When only using compact branches if { $isa_rev < 6 - && ([mips_have_test_option_p options "HAS_LSA"] - || [mips_have_test_option_p options "-mcompact-branches=always"]) } { + && ([mips_have_test_option_p options "HAS_LSA"]) } { if { $gp_size == 32 } { mips_make_test_option options "-mips32r6" } else {
For MIPSr6, we may wish to use compact-branches only. Currently, we have to use `always' option, while it is mark as conflict with pre-R6. cc1: error: unsupported combination: ‘mips32r2’ -mcompact-branches=always Just ignore -mcompact-branches=always for pre-R6. This patch also defines __mips_compact_branches_never __mips_compact_branches_always __mips_compact_branches_optimal predefined macros gcc/ChangeLog: * config/mips/mips.c (mips_option_override): * config/mips/mips.h (TARGET_RTP_PIC): not trigger error for compact-branches=always for pre-R6. (TARGET_CB_NEVER): Likewise. (TARGET_CB_ALWAYS): Likewise. (struct mips_cpu_info): define macros for compact branch policy. * doc/invoke.texi: Document "always" with pre-R6. gcc/testsuite/ChangeLog: * gcc.target/mips/compact-branches-1.c: add isa_rev>=6. * gcc.target/mips/mips.exp: don't add -mipsXXr6 option for -mcompact-branches=always. It is usable for pre-R6 now. * gcc.target/mips/compact-branches-8.c: New test. * gcc.target/mips/compact-branches-9.c: New test. --- gcc/config/mips/mips.c | 8 +------ gcc/config/mips/mips.h | 22 ++++++++++++------- gcc/doc/invoke.texi | 1 + .../gcc.target/mips/compact-branches-1.c | 2 +- .../gcc.target/mips/compact-branches-8.c | 10 +++++++++ .../gcc.target/mips/compact-branches-9.c | 10 +++++++++ gcc/testsuite/gcc.target/mips/mips.exp | 4 +--- 7 files changed, 38 insertions(+), 19 deletions(-) create mode 100644 gcc/testsuite/gcc.target/mips/compact-branches-8.c create mode 100644 gcc/testsuite/gcc.target/mips/compact-branches-9.c