| Message ID | 000001cf584e$66102060$32306120$@arm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 15, 2014 at 3:59 AM, Joey Ye <joey.ye@arm.com> wrote: > If-converstion is harmful to optimized debugging as it generates conditional > execution instructions with line number information, which resulted in a > dillusion to developers that both then-else branches are executed. > > For example: > test.c: > 1: unsigned oldest_sequence; > 2: > 3: unsigned foo(unsigned seq_number) > 4: { > 5: if ((seq_number + 5) < 10) > 6: seq_number += 100; > 7: else > 8: seq_number = oldest_sequence; > > if (seq_number < oldest_sequence) > seq_number = oldest_sequence; > > return seq_number; > } > > $ arm-none-eabi-gcc -mthumb -mcpu=cortex-m3 -Og -g3 > gets: > .loc 1 5 0 > adds r3, r0, #5 > cmp r3, #9 > .loc 1 6 0 <----- line 6, then branch > itee ls > addls r0, r0, #100 > .LVL1: > .loc 1 8 0 <----- line 8, else branch. Both branches seems to > be executed in GDB > ldrhi r3, .L5 > ldrhi r0, [r3] > > The reason is that if_conversion2 is still enabled in Og. The patch simply > disables it for Og. > > Tests: > * -Og bootstrap passed. > * Make check default (no additional option): No regression. > * Make check with -Og: expected regressions. Cases relying on if-conversion2 > failed. >> FAIL: gcc.target/arm/its.c scan-assembler-times \\tit 2 >> FAIL: gcc.target/arm/pr40956.c scan-assembler-times mov[\\\\t ]*r., #0 1 >> FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq >> FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne >> FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler asrne >> FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler lslne > > OK to trunk and 4.8/4.9 branch? Ok for trunk and branches after a while. Why does if-conversion not have the same problem? On the GIMPLE part we avoid all kinds of if-conversion with -Og. Thanks, Richard. > ChangeLog: > * opts.c (OPT_fif_conversion2): Disable for Og. > > diff --git a/gcc/opts.c b/gcc/opts.c > index fdc903f..e076253 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -432,7 +432,7 @@ static const struct default_options > default_options_table[] = > { OPT_LEVELS_1_PLUS, OPT_fcprop_registers, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fforward_propagate, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fif_conversion, NULL, 1 }, > - { OPT_LEVELS_1_PLUS, OPT_fif_conversion2, NULL, 1 }, > + { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fif_conversion2, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fipa_pure_const, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fipa_reference, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fipa_profile, NULL, 1 }, > >
> -----Original Message----- > From: Richard Biener [mailto:richard.guenther@gmail.com] > Sent: Tuesday, April 15, 2014 4:05 PM > To: Joey Ye > Cc: GCC Patches > Subject: Re: [patch] Disable if_conversion2 for Og > > On Tue, Apr 15, 2014 at 3:59 AM, Joey Ye <joey.ye@arm.com> wrote: > > If-converstion is harmful to optimized debugging as it generates > > conditional execution instructions with line number information, which > > resulted in a dillusion to developers that both then-else branches are > executed. > > > > For example: > > test.c: > > 1: unsigned oldest_sequence; > > 2: > > 3: unsigned foo(unsigned seq_number) > > 4: { > > 5: if ((seq_number + 5) < 10) > > 6: seq_number += 100; > > 7: else > > 8: seq_number = oldest_sequence; > > > > if (seq_number < oldest_sequence) > > seq_number = oldest_sequence; > > > > return seq_number; > > } > > > > $ arm-none-eabi-gcc -mthumb -mcpu=cortex-m3 -Og -g3 > > gets: > > .loc 1 5 0 > > adds r3, r0, #5 > > cmp r3, #9 > > .loc 1 6 0 <----- line 6, then branch > > itee ls > > addls r0, r0, #100 > > .LVL1: > > .loc 1 8 0 <----- line 8, else branch. Both branches seems to > > be executed in GDB > > ldrhi r3, .L5 > > ldrhi r0, [r3] > > > > The reason is that if_conversion2 is still enabled in Og. The patch > > simply disables it for Og. > > > > Tests: > > * -Og bootstrap passed. > > * Make check default (no additional option): No regression. > > * Make check with -Og: expected regressions. Cases relying on > > if-conversion2 failed. > >> FAIL: gcc.target/arm/its.c scan-assembler-times \\tit 2 > >> FAIL: gcc.target/arm/pr40956.c scan-assembler-times mov[\\\\t ]*r., > >> #0 1 > >> FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq > >> FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne > >> FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler asrne > >> FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler lslne > > > > OK to trunk and 4.8/4.9 branch? > > Ok for trunk and branches after a while. Why does if-conversion not have > the same problem? On the GIMPLE part we avoid all kinds of if-conversion > with -Og. I think if-conversion should be disabled for Og too, but I don't have a case to show that it is harmful. If GIMPLE avoids all if-conversion, it is nature to do the same for RTL. I'll test and send another patch to also disable if-conversion.
On 15/04/14 02:59, Joey Ye wrote: > If-converstion is harmful to optimized debugging as it generates conditional > execution instructions with line number information, which resulted in a > dillusion to developers that both then-else branches are executed. > > For example: > test.c: > 1: unsigned oldest_sequence; > 2: > 3: unsigned foo(unsigned seq_number) > 4: { > 5: if ((seq_number + 5) < 10) > 6: seq_number += 100; > 7: else > 8: seq_number = oldest_sequence; > > if (seq_number < oldest_sequence) > seq_number = oldest_sequence; > > return seq_number; > } Arguably, this is a bug in gdb. The debugger should understand when a breakpointed conditional instruction is not going to execute and silently continue. That preserves the illusion of not executing the code without requiring the compiler to de-optimize things. R. > > $ arm-none-eabi-gcc -mthumb -mcpu=cortex-m3 -Og -g3 > gets: > .loc 1 5 0 > adds r3, r0, #5 > cmp r3, #9 > .loc 1 6 0 <----- line 6, then branch > itee ls > addls r0, r0, #100 > .LVL1: > .loc 1 8 0 <----- line 8, else branch. Both branches seems to > be executed in GDB > ldrhi r3, .L5 > ldrhi r0, [r3] > > The reason is that if_conversion2 is still enabled in Og. The patch simply > disables it for Og. > > Tests: > * -Og bootstrap passed. > * Make check default (no additional option): No regression. > * Make check with -Og: expected regressions. Cases relying on if-conversion2 > failed. >> FAIL: gcc.target/arm/its.c scan-assembler-times \\tit 2 >> FAIL: gcc.target/arm/pr40956.c scan-assembler-times mov[\\\\t ]*r., #0 1 >> FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq >> FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne >> FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler asrne >> FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler lslne > > OK to trunk and 4.8/4.9 branch? > > ChangeLog: > * opts.c (OPT_fif_conversion2): Disable for Og. > > diff --git a/gcc/opts.c b/gcc/opts.c > index fdc903f..e076253 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -432,7 +432,7 @@ static const struct default_options > default_options_table[] = > { OPT_LEVELS_1_PLUS, OPT_fcprop_registers, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fforward_propagate, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fif_conversion, NULL, 1 }, > - { OPT_LEVELS_1_PLUS, OPT_fif_conversion2, NULL, 1 }, > + { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fif_conversion2, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fipa_pure_const, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fipa_reference, NULL, 1 }, > { OPT_LEVELS_1_PLUS, OPT_fipa_profile, NULL, 1 }, > > >
> -----Original Message----- > From: Richard Earnshaw > Sent: Wednesday, April 16, 2014 5:44 PM > To: Joey Ye > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [patch] Disable if_conversion2 for Og > > Arguably, this is a bug in gdb. The debugger should understand when a > breakpointed conditional instruction is not going to execute and silently > continue. That preserves the illusion of not executing the code without > requiring the compiler to de-optimize things. > > R. Or compiler just optimizes it, and emits generic DWARFx information to help GDB handle it in more target independently? - Joey
On 16/04/14 11:02, Joey Ye wrote: > > >> -----Original Message----- >> From: Richard Earnshaw >> Sent: Wednesday, April 16, 2014 5:44 PM >> To: Joey Ye >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [patch] Disable if_conversion2 for Og >> >> Arguably, this is a bug in gdb. The debugger should understand when a >> breakpointed conditional instruction is not going to execute and silently >> continue. That preserves the illusion of not executing the code without >> requiring the compiler to de-optimize things. >> >> R. > Or compiler just optimizes it, and emits generic DWARFx information to help > GDB handle it in more target independently? > > - Joey > I'm not sure extra dwarf info would help much. The debugger still has to understand that the breakpoint has not really been hit. R.
On 16/04/14 11:17, Joey Ye wrote: >> -----Original Message----- >> From: Richard Earnshaw >> Sent: Wednesday, April 16, 2014 6:04 PM >> To: Joey Ye >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [patch] Disable if_conversion2 for Og >> >> On 16/04/14 11:02, Joey Ye wrote: >>> >>> >>>> -----Original Message----- >>>> From: Richard Earnshaw >>>> Sent: Wednesday, April 16, 2014 5:44 PM >>>> To: Joey Ye >>>> Cc: gcc-patches@gcc.gnu.org >>>> Subject: Re: [patch] Disable if_conversion2 for Og >>>> >>>> Arguably, this is a bug in gdb. The debugger should understand when >>>> a breakpointed conditional instruction is not going to execute and >>>> silently continue. That preserves the illusion of not executing the >>>> code without requiring the compiler to de-optimize things. >>>> >>>> R. >>> Or compiler just optimizes it, and emits generic DWARFx information to >>> help GDB handle it in more target independently? >>> >>> - Joey >>> >> >> I'm not sure extra dwarf info would help much. The debugger still has to >> understand that the breakpoint has not really been hit. >> >> R. > Yes, it is inevitable. But without extra dwarf info it will be even more painful: each time setting break-point or break-point hits it has to decode the break-pointed instructions and its context to search for conditional execution and IT blocks. > For thumb code it can get the conditional information it needs from the IT state in the PSR; for ARM code it has to look no further than the instruction itself. R.
> -----Original Message----- > From: Richard Earnshaw > Sent: Wednesday, April 16, 2014 6:04 PM > To: Joey Ye > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [patch] Disable if_conversion2 for Og > > On 16/04/14 11:02, Joey Ye wrote: > > > > > >> -----Original Message----- > >> From: Richard Earnshaw > >> Sent: Wednesday, April 16, 2014 5:44 PM > >> To: Joey Ye > >> Cc: gcc-patches@gcc.gnu.org > >> Subject: Re: [patch] Disable if_conversion2 for Og > >> > >> Arguably, this is a bug in gdb. The debugger should understand when > >> a breakpointed conditional instruction is not going to execute and > >> silently continue. That preserves the illusion of not executing the > >> code without requiring the compiler to de-optimize things. > >> > >> R. > > Or compiler just optimizes it, and emits generic DWARFx information to > > help GDB handle it in more target independently? > > > > - Joey > > > > I'm not sure extra dwarf info would help much. The debugger still has to > understand that the breakpoint has not really been hit. > > R. Yes, it is inevitable. But without extra dwarf info it will be even more painful: each time setting break-point or break-point hits it has to decode the break-pointed instructions and its context to search for conditional execution and IT blocks. - Joey
> -----Original Message----- > From: Richard Earnshaw > Sent: Wednesday, April 16, 2014 6:21 PM > To: Joey Ye > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [patch] Disable if_conversion2 for Og > > On 16/04/14 11:17, Joey Ye wrote: > >> -----Original Message----- > >> From: Richard Earnshaw > >> Sent: Wednesday, April 16, 2014 6:04 PM > >> To: Joey Ye > >> Cc: gcc-patches@gcc.gnu.org > >> Subject: Re: [patch] Disable if_conversion2 for Og > >> > >> On 16/04/14 11:02, Joey Ye wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Richard Earnshaw > >>>> Sent: Wednesday, April 16, 2014 5:44 PM > >>>> To: Joey Ye > >>>> Cc: gcc-patches@gcc.gnu.org > >>>> Subject: Re: [patch] Disable if_conversion2 for Og > >>>> > >>>> Arguably, this is a bug in gdb. The debugger should understand > >>>> when a breakpointed conditional instruction is not going to execute > >>>> and silently continue. That preserves the illusion of not > >>>> executing the code without requiring the compiler to de-optimize > things. > >>>> > >>>> R. > >>> Or compiler just optimizes it, and emits generic DWARFx information > >>> to help GDB handle it in more target independently? > >>> > >>> - Joey > >>> > >> > >> I'm not sure extra dwarf info would help much. The debugger still > >> has to understand that the breakpoint has not really been hit. > >> > >> R. > > Yes, it is inevitable. But without extra dwarf info it will be even more painful: > each time setting break-point or break-point hits it has to decode the break- > pointed instructions and its context to search for conditional execution and IT > blocks. > > > > For thumb code it can get the conditional information it needs from the IT > state in the PSR; for ARM code it has to look no further than the instruction > itself. > > R. The thing is, debugger has to do this for every breakpoint, even though more of them are not conditional execution, which isn't efficient.
On 16/04/14 11:30, Joey Ye wrote: > > >> -----Original Message----- >> From: Richard Earnshaw >> Sent: Wednesday, April 16, 2014 6:21 PM >> To: Joey Ye >> Cc: gcc-patches@gcc.gnu.org >> Subject: Re: [patch] Disable if_conversion2 for Og >> >> On 16/04/14 11:17, Joey Ye wrote: >>>> -----Original Message----- >>>> From: Richard Earnshaw >>>> Sent: Wednesday, April 16, 2014 6:04 PM >>>> To: Joey Ye >>>> Cc: gcc-patches@gcc.gnu.org >>>> Subject: Re: [patch] Disable if_conversion2 for Og >>>> >>>> On 16/04/14 11:02, Joey Ye wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Richard Earnshaw >>>>>> Sent: Wednesday, April 16, 2014 5:44 PM >>>>>> To: Joey Ye >>>>>> Cc: gcc-patches@gcc.gnu.org >>>>>> Subject: Re: [patch] Disable if_conversion2 for Og >>>>>> >>>>>> Arguably, this is a bug in gdb. The debugger should understand >>>>>> when a breakpointed conditional instruction is not going to execute >>>>>> and silently continue. That preserves the illusion of not >>>>>> executing the code without requiring the compiler to de-optimize >> things. >>>>>> >>>>>> R. >>>>> Or compiler just optimizes it, and emits generic DWARFx information >>>>> to help GDB handle it in more target independently? >>>>> >>>>> - Joey >>>>> >>>> >>>> I'm not sure extra dwarf info would help much. The debugger still >>>> has to understand that the breakpoint has not really been hit. >>>> >>>> R. >>> Yes, it is inevitable. But without extra dwarf info it will be even more > painful: >> each time setting break-point or break-point hits it has to decode the > break- >> pointed instructions and its context to search for conditional execution > and IT >> blocks. >>> >> >> For thumb code it can get the conditional information it needs from the IT >> state in the PSR; for ARM code it has to look no further than the > instruction >> itself. >> >> R. > The thing is, debugger has to do this for every breakpoint, even though more > of them are not conditional execution, which isn't efficient. > > Then cache whether the instruction might be conditional when it's created. That's more work when creating the bp (and you do have to scan back to check if a thumb instruction might be made conditional with IT), but would save time when hitting it. Anyway, this is getting off-topic for the GCC-patches list. R.
diff --git a/gcc/opts.c b/gcc/opts.c index fdc903f..e076253 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -432,7 +432,7 @@ static const struct default_options default_options_table[] = { OPT_LEVELS_1_PLUS, OPT_fcprop_registers, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fforward_propagate, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fif_conversion, NULL, 1 }, - { OPT_LEVELS_1_PLUS, OPT_fif_conversion2, NULL, 1 }, + { OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fif_conversion2, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fipa_pure_const, NULL, 1 }, { OPT_LEVELS_1_PLUS, OPT_fipa_reference, NULL, 1 },
If-converstion is harmful to optimized debugging as it generates conditional execution instructions with line number information, which resulted in a dillusion to developers that both then-else branches are executed. For example: test.c: 1: unsigned oldest_sequence; 2: 3: unsigned foo(unsigned seq_number) 4: { 5: if ((seq_number + 5) < 10) 6: seq_number += 100; 7: else 8: seq_number = oldest_sequence; if (seq_number < oldest_sequence) seq_number = oldest_sequence; return seq_number; } $ arm-none-eabi-gcc -mthumb -mcpu=cortex-m3 -Og -g3 gets: .loc 1 5 0 adds r3, r0, #5 cmp r3, #9 .loc 1 6 0 <----- line 6, then branch itee ls addls r0, r0, #100 .LVL1: .loc 1 8 0 <----- line 8, else branch. Both branches seems to be executed in GDB ldrhi r3, .L5 ldrhi r0, [r3] The reason is that if_conversion2 is still enabled in Og. The patch simply disables it for Og. Tests: * -Og bootstrap passed. * Make check default (no additional option): No regression. * Make check with -Og: expected regressions. Cases relying on if-conversion2 failed. > FAIL: gcc.target/arm/its.c scan-assembler-times \\tit 2 > FAIL: gcc.target/arm/pr40956.c scan-assembler-times mov[\\\\t ]*r., #0 1 > FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler asreq > FAIL: gcc.target/arm/thumb-ifcvt-2.c scan-assembler lslne > FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler asrne > FAIL: gcc.target/arm/thumb-ifcvt.c scan-assembler lslne OK to trunk and 4.8/4.9 branch? ChangeLog: * opts.c (OPT_fif_conversion2): Disable for Og. { OPT_LEVELS_1_PLUS, OPT_fipa_profile, NULL, 1 },