* [RFC][debug] Add -fadd-debug-nops
@ 2018-07-16 13:29 Tom de Vries
2018-07-16 13:34 ` Jakub Jelinek
2018-07-16 13:50 ` Richard Biener
0 siblings, 2 replies; 23+ messages in thread
From: Tom de Vries @ 2018-07-16 13:29 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Biener, Jakub Jelinek, Jim Wilson
Hi,
this is an idea that I'm currently playing around with: adding nops in
an optimized application with debug info can improve the debug info.
Consider f.i. this gdb session in foo of pr54200-2.c (using -Os):
...
(gdb) n
26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
(gdb) p a
'a' has unknown type; cast it to its declared type
(gdb) n
main () at pr54200-2.c:34
34 return 0;
...
The problem is that the scope in which a is declared ends at .LBE7, and the
statement .loc for line 26 ends up attached to the ret insn:
...
.loc 1 24 11
addl %edx, %eax
.LVL1:
# DEBUG a => ax
.loc 1 26 7 is_stmt 1
.LBE7:
.loc 1 28 1 is_stmt 0
ret
.cfi_endproc
...
This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing
for O1 and higher) by adding a nop before the ret insn:
...
.loc 1 24 11
addl %edx, %eax
.LVL1:
# DEBUG a => ax
.loc 1 26 7 is_stmt 1
+ nop
.LBE7:
.loc 1 28 1 is_stmt 0
ret
.cfi_endproc
...
and instead we have:
...
(gdb) n
26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
(gdb) p a
$1 = 6
(gdb) n
main () at pr54200-2.c:34
34 return 0;
...
Any comments?
Thanks,
- Tom
[debug] Add -fadd-debug-nops
---
gcc/common.opt | 4 ++++
gcc/testsuite/gcc.dg/guality/pr54200-2.c | 37 ++++++++++++++++++++++++++++++++
gcc/var-tracking.c | 16 ++++++++++++++
3 files changed, 57 insertions(+)
diff --git a/gcc/common.opt b/gcc/common.opt
index c29abdb5cb1..8e2876d2b8e 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1504,6 +1504,10 @@ Common Report Var(flag_hoist_adjacent_loads) Optimization
Enable hoisting adjacent loads to encourage generating conditional move
instructions.
+fadd-debug-nops
+Common Report Var(flag_add_debug_nops) Optimization
+Add nops if that improves debugging of optimized code
+
fkeep-gc-roots-live
Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
; Always keep a pointer to a live memory block
diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
new file mode 100644
index 00000000000..2694f7401e2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
@@ -0,0 +1,37 @@
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
+/* { dg-options "-g -fadd-debug-nops -DMAIN" } */
+
+#include "prevent-optimization.h"
+
+int o ATTRIBUTE_USED;
+
+void
+bar (void)
+{
+ o = 2;
+}
+
+int __attribute__((noinline,noclone))
+foo (int z, int x, int b)
+{
+ if (x == 1)
+ {
+ bar ();
+ return z;
+ }
+ else
+ {
+ int a = (x + z) + b;
+ /* Add cast to prevent unsupported. */
+ return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
+ }
+}
+
+#ifdef MAIN
+int main ()
+{
+ foo (3, 2, 1);
+ return 0;
+}
+#endif
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 5537fa64085..230845a38a0 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -9994,6 +9994,15 @@ reemit_marker_as_note (rtx_insn *insn)
}
}
+static void
+emit_nop_before (rtx_insn *insn)
+{
+ rtx_insn *insert_after = PREV_INSN (insn);
+ while (INSN_LOCATION (insert_after) == INSN_LOCATION (insn))
+ insert_after = PREV_INSN (insert_after);
+ emit_insn_after (gen_nop (), insert_after);
+}
+
/* Allocate and initialize the data structures for variable tracking
and parse the RTL to get the micro operations. */
@@ -10224,6 +10233,13 @@ vt_initialize (void)
continue;
}
+ /* Add debug nops before ret insn. */
+ if (optimize
+ && flag_add_debug_nops
+ && cfun->debug_nonbind_markers
+ && returnjump_p (insn))
+ emit_nop_before (insn);
+
if (MAY_HAVE_DEBUG_BIND_INSNS)
{
if (CALL_P (insn))
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC][debug] Add -fadd-debug-nops
2018-07-16 13:29 [RFC][debug] Add -fadd-debug-nops Tom de Vries
@ 2018-07-16 13:34 ` Jakub Jelinek
2018-07-16 14:32 ` Tom de Vries
2018-07-16 13:50 ` Richard Biener
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Jelinek @ 2018-07-16 13:34 UTC (permalink / raw)
To: Tom de Vries, Alexandre Oliva; +Cc: gcc-patches, Richard Biener, Jim Wilson
On Mon, Jul 16, 2018 at 03:29:10PM +0200, Tom de Vries wrote:
> this is an idea that I'm currently playing around with: adding nops in
> an optimized application with debug info can improve the debug info.
>
> Consider f.i. this gdb session in foo of pr54200-2.c (using -Os):
> ...
> (gdb) n
> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
> (gdb) p a
> 'a' has unknown type; cast it to its declared type
> (gdb) n
> main () at pr54200-2.c:34
> 34 return 0;
> ...
>
> The problem is that the scope in which a is declared ends at .LBE7, and the
> statement .loc for line 26 ends up attached to the ret insn:
> ...
> .loc 1 24 11
> addl %edx, %eax
> .LVL1:
> # DEBUG a => ax
> .loc 1 26 7 is_stmt 1
> .LBE7:
> .loc 1 28 1 is_stmt 0
> ret
> .cfi_endproc
> ...
>
> This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing
> for O1 and higher) by adding a nop before the ret insn:
> ...
> .loc 1 24 11
> addl %edx, %eax
> .LVL1:
> # DEBUG a => ax
> .loc 1 26 7 is_stmt 1
> + nop
> .LBE7:
> .loc 1 28 1 is_stmt 0
> ret
> .cfi_endproc
> ...
>
> and instead we have:
> ...
> (gdb) n
> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
> (gdb) p a
> $1 = 6
> (gdb) n
> main () at pr54200-2.c:34
> 34 return 0;
> ...
>
> Any comments?
So is this essentially a workaround for GDB not supporting the statement
frontiers? Isn't the right fix just to add that support instead and then
users can choose how exactly they want to step through the function in the
debugger.
Jakub
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC][debug] Add -fadd-debug-nops
2018-07-16 13:29 [RFC][debug] Add -fadd-debug-nops Tom de Vries
2018-07-16 13:34 ` Jakub Jelinek
@ 2018-07-16 13:50 ` Richard Biener
2018-07-16 15:10 ` Tom de Vries
1 sibling, 1 reply; 23+ messages in thread
From: Richard Biener @ 2018-07-16 13:50 UTC (permalink / raw)
To: Tom de Vries; +Cc: gcc-patches, Jakub Jelinek, Jim Wilson, aoliva
On Mon, 16 Jul 2018, Tom de Vries wrote:
> Hi,
>
> this is an idea that I'm currently playing around with: adding nops in
> an optimized application with debug info can improve the debug info.
>
>
> Consider f.i. this gdb session in foo of pr54200-2.c (using -Os):
> ...
> (gdb) n
> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
> (gdb) p a
> 'a' has unknown type; cast it to its declared type
> (gdb) n
> main () at pr54200-2.c:34
> 34 return 0;
> ...
>
> The problem is that the scope in which a is declared ends at .LBE7, and the
> statement .loc for line 26 ends up attached to the ret insn:
> ...
> .loc 1 24 11
> addl %edx, %eax
> .LVL1:
> # DEBUG a => ax
> .loc 1 26 7 is_stmt 1
> .LBE7:
> .loc 1 28 1 is_stmt 0
> ret
> .cfi_endproc
> ...
>
> This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing
> for O1 and higher) by adding a nop before the ret insn:
> ...
> .loc 1 24 11
> addl %edx, %eax
> .LVL1:
> # DEBUG a => ax
> .loc 1 26 7 is_stmt 1
> + nop
> .LBE7:
> .loc 1 28 1 is_stmt 0
> ret
> .cfi_endproc
> ...
>
> and instead we have:
> ...
> (gdb) n
> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
> (gdb) p a
> $1 = 6
> (gdb) n
> main () at pr54200-2.c:34
> 34 return 0;
> ...
>
> Any comments?
Interesting idea. I wonder if that should be generalized
to other places and how we can avoid compare-debug failures
(var-tracking usually doesn't change code-generation).
Richard.
> Thanks,
> - Tom
>
> [debug] Add -fadd-debug-nops
>
> ---
> gcc/common.opt | 4 ++++
> gcc/testsuite/gcc.dg/guality/pr54200-2.c | 37 ++++++++++++++++++++++++++++++++
> gcc/var-tracking.c | 16 ++++++++++++++
> 3 files changed, 57 insertions(+)
>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index c29abdb5cb1..8e2876d2b8e 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1504,6 +1504,10 @@ Common Report Var(flag_hoist_adjacent_loads) Optimization
> Enable hoisting adjacent loads to encourage generating conditional move
> instructions.
>
> +fadd-debug-nops
> +Common Report Var(flag_add_debug_nops) Optimization
> +Add nops if that improves debugging of optimized code
> +
> fkeep-gc-roots-live
> Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
> ; Always keep a pointer to a live memory block
> diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> new file mode 100644
> index 00000000000..2694f7401e2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> @@ -0,0 +1,37 @@
> +/* { dg-do run } */
> +/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
> +/* { dg-options "-g -fadd-debug-nops -DMAIN" } */
> +
> +#include "prevent-optimization.h"
> +
> +int o ATTRIBUTE_USED;
> +
> +void
> +bar (void)
> +{
> + o = 2;
> +}
> +
> +int __attribute__((noinline,noclone))
> +foo (int z, int x, int b)
> +{
> + if (x == 1)
> + {
> + bar ();
> + return z;
> + }
> + else
> + {
> + int a = (x + z) + b;
> + /* Add cast to prevent unsupported. */
> + return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
> + }
> +}
> +
> +#ifdef MAIN
> +int main ()
> +{
> + foo (3, 2, 1);
> + return 0;
> +}
> +#endif
> diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
> index 5537fa64085..230845a38a0 100644
> --- a/gcc/var-tracking.c
> +++ b/gcc/var-tracking.c
> @@ -9994,6 +9994,15 @@ reemit_marker_as_note (rtx_insn *insn)
> }
> }
>
> +static void
> +emit_nop_before (rtx_insn *insn)
> +{
> + rtx_insn *insert_after = PREV_INSN (insn);
> + while (INSN_LOCATION (insert_after) == INSN_LOCATION (insn))
> + insert_after = PREV_INSN (insert_after);
> + emit_insn_after (gen_nop (), insert_after);
> +}
> +
> /* Allocate and initialize the data structures for variable tracking
> and parse the RTL to get the micro operations. */
>
> @@ -10224,6 +10233,13 @@ vt_initialize (void)
> continue;
> }
>
> + /* Add debug nops before ret insn. */
> + if (optimize
> + && flag_add_debug_nops
> + && cfun->debug_nonbind_markers
> + && returnjump_p (insn))
> + emit_nop_before (insn);
> +
> if (MAY_HAVE_DEBUG_BIND_INSNS)
> {
> if (CALL_P (insn))
>
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC][debug] Add -fadd-debug-nops
2018-07-16 13:34 ` Jakub Jelinek
@ 2018-07-16 14:32 ` Tom de Vries
2018-07-17 1:02 ` Alexandre Oliva
0 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2018-07-16 14:32 UTC (permalink / raw)
To: Jakub Jelinek, Alexandre Oliva; +Cc: gcc-patches, Richard Biener, Jim Wilson
On 07/16/2018 03:34 PM, Jakub Jelinek wrote:
> On Mon, Jul 16, 2018 at 03:29:10PM +0200, Tom de Vries wrote:
>> this is an idea that I'm currently playing around with: adding nops in
>> an optimized application with debug info can improve the debug info.
>>
>> Consider f.i. this gdb session in foo of pr54200-2.c (using -Os):
>> ...
>> (gdb) n
>> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
>> (gdb) p a
>> 'a' has unknown type; cast it to its declared type
>> (gdb) n
>> main () at pr54200-2.c:34
>> 34 return 0;
>> ...
>>
>> The problem is that the scope in which a is declared ends at .LBE7, and the
>> statement .loc for line 26 ends up attached to the ret insn:
>> ...
>> .loc 1 24 11
>> addl %edx, %eax
>> .LVL1:
>> # DEBUG a => ax
>> .loc 1 26 7 is_stmt 1
>> .LBE7:
>> .loc 1 28 1 is_stmt 0
>> ret
>> .cfi_endproc
>> ...
>>
>> This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing
>> for O1 and higher) by adding a nop before the ret insn:
>> ...
>> .loc 1 24 11
>> addl %edx, %eax
>> .LVL1:
>> # DEBUG a => ax
>> .loc 1 26 7 is_stmt 1
>> + nop
>> .LBE7:
>> .loc 1 28 1 is_stmt 0
>> ret
>> .cfi_endproc
>> ...
>>
>> and instead we have:
>> ...
>> (gdb) n
>> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
>> (gdb) p a
>> $1 = 6
>> (gdb) n
>> main () at pr54200-2.c:34
>> 34 return 0;
>> ...
>>
>> Any comments?
>
> So is this essentially a workaround for GDB not supporting the statement
> frontiers?
AFAIU now, the concept of location views addresses this problem, so yes.
> Isn't the right fix just to add that support instead and then
> users can choose how exactly they want to step through the function in the
> debugger.
Right, but in the mean time I don't mind having an option that lets me
filter out noise in guality test results.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC][debug] Add -fadd-debug-nops
2018-07-16 13:50 ` Richard Biener
@ 2018-07-16 15:10 ` Tom de Vries
2018-07-24 11:30 ` Tom de Vries
0 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2018-07-16 15:10 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek, Jim Wilson, aoliva
On 07/16/2018 03:50 PM, Richard Biener wrote:
> On Mon, 16 Jul 2018, Tom de Vries wrote:
>
>> Hi,
>>
>> this is an idea that I'm currently playing around with: adding nops in
>> an optimized application with debug info can improve the debug info.
>>
>>
>> Consider f.i. this gdb session in foo of pr54200-2.c (using -Os):
>> ...
>> (gdb) n
>> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
>> (gdb) p a
>> 'a' has unknown type; cast it to its declared type
>> (gdb) n
>> main () at pr54200-2.c:34
>> 34 return 0;
>> ...
>>
>> The problem is that the scope in which a is declared ends at .LBE7, and the
>> statement .loc for line 26 ends up attached to the ret insn:
>> ...
>> .loc 1 24 11
>> addl %edx, %eax
>> .LVL1:
>> # DEBUG a => ax
>> .loc 1 26 7 is_stmt 1
>> .LBE7:
>> .loc 1 28 1 is_stmt 0
>> ret
>> .cfi_endproc
>> ...
>>
>> This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing
>> for O1 and higher) by adding a nop before the ret insn:
>> ...
>> .loc 1 24 11
>> addl %edx, %eax
>> .LVL1:
>> # DEBUG a => ax
>> .loc 1 26 7 is_stmt 1
>> + nop
>> .LBE7:
>> .loc 1 28 1 is_stmt 0
>> ret
>> .cfi_endproc
>> ...
>>
>> and instead we have:
>> ...
>> (gdb) n
>> 26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
>> (gdb) p a
>> $1 = 6
>> (gdb) n
>> main () at pr54200-2.c:34
>> 34 return 0;
>> ...
>>
>> Any comments?
>
> Interesting idea. I wonder if that should be generalized
> to other places
I kept the option name general, to allow for that.
And indeed, this is a point-fix patch. I've been playing around with a
more generic patch that adds nops such that each is_stmt .loc is
associated with a unique insn, but that was embedded in an
fkeep-vars-live branch, so this patch is minimally addressing the first
problem I managed to reproduce on trunk.
> and how we can avoid compare-debug failures
> (var-tracking usually doesn't change code-generation).
>
I could remove the cfun->debug_nonbind_markers test and move the
nop-insertion to a separate pass or some such.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC][debug] Add -fadd-debug-nops
2018-07-16 14:32 ` Tom de Vries
@ 2018-07-17 1:02 ` Alexandre Oliva
0 siblings, 0 replies; 23+ messages in thread
From: Alexandre Oliva @ 2018-07-17 1:02 UTC (permalink / raw)
To: Tom de Vries; +Cc: Jakub Jelinek, gcc-patches, Richard Biener, Jim Wilson
On Jul 16, 2018, Tom de Vries <tdevries@suse.de> wrote:
> On 07/16/2018 03:34 PM, Jakub Jelinek wrote:
>> So is this essentially a workaround for GDB not supporting the statement
>> frontiers?
> AFAIU now, the concept of location views addresses this problem, so yes.
Nice! A preview for what can be obtained with LVu support in the
debugger!
> Right, but in the mean time I don't mind having an option that lets me
> filter out noise in guality test results.
FWIW, I'm a bit concerned about working around legitimate problems, as
in modifying testcases so that they pass. This hides actual problems,
that we'd like to fix eventually by adjusting the compiler, not the
testcases.
That said, thank you for the attention you've given to the guality
testsuite recently. It's appreciated.
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain Engineer Free Software Evangelist
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC][debug] Add -fadd-debug-nops
2018-07-16 15:10 ` Tom de Vries
@ 2018-07-24 11:30 ` Tom de Vries
2018-07-24 11:35 ` [RFC 1/3, debug] Add fdebug-nops Tom de Vries
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 11:30 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek, Jim Wilson, aoliva
On 07/16/2018 05:10 PM, Tom de Vries wrote:
> On 07/16/2018 03:50 PM, Richard Biener wrote:
>> On Mon, 16 Jul 2018, Tom de Vries wrote:
>>> Any comments?
>>
>> Interesting idea. I wonder if that should be generalized
>> to other places
>
> I kept the option name general, to allow for that.
>
> And indeed, this is a point-fix patch. I've been playing around with a
> more generic patch that adds nops such that each is_stmt .loc is
> associated with a unique insn, but that was embedded in an
> fkeep-vars-live branch, so this patch is minimally addressing the first
> problem I managed to reproduce on trunk.
>
>> and how we can avoid compare-debug failures
>> (var-tracking usually doesn't change code-generation).
>>
>
I'll post this patch series (the current state of my fkeep-vars-live
branch) in reply to this email:
1 [debug] Add fdebug-nops
2 [debug] Add fkeep-vars-live
3 [debug] Add fdebug-nops and fkeep-vars-live to Og only
Bootstrapped and reg-tested on x86_64. ChangeLog entries and function
header comments missing.
Comments welcome.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC 1/3, debug] Add fdebug-nops
2018-07-24 11:30 ` Tom de Vries
@ 2018-07-24 11:35 ` Tom de Vries
2018-07-24 14:59 ` [PATCH, " Tom de Vries
2018-07-24 19:07 ` [RFC 1/3, " Alexandre Oliva
2018-07-24 11:37 ` [RFC 2/3, debug] Add fkeep-vars-live Tom de Vries
2018-07-24 11:38 ` [RFC 3/3, debug] Add fdebug-nops and fkeep-vars-live to Og only Tom de Vries
2 siblings, 2 replies; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 11:35 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek, Jim Wilson, aoliva
On Tue, Jul 24, 2018 at 01:30:30PM +0200, Tom de Vries wrote:
> On 07/16/2018 05:10 PM, Tom de Vries wrote:
> > On 07/16/2018 03:50 PM, Richard Biener wrote:
> >> On Mon, 16 Jul 2018, Tom de Vries wrote:
> >>> Any comments?
> >>
> >> Interesting idea. I wonder if that should be generalized
> >> to other places
> >
> > I kept the option name general, to allow for that.
> >
> > And indeed, this is a point-fix patch. I've been playing around with a
> > more generic patch that adds nops such that each is_stmt .loc is
> > associated with a unique insn, but that was embedded in an
> > fkeep-vars-live branch, so this patch is minimally addressing the first
> > problem I managed to reproduce on trunk.
> >
> >> and how we can avoid compare-debug failures
> >> (var-tracking usually doesn't change code-generation).
> >>
> >
>
> I'll post this patch series (the current state of my fkeep-vars-live
> branch) in reply to this email:
>
> 1 [debug] Add fdebug-nops
> 2 [debug] Add fkeep-vars-live
> 3 [debug] Add fdebug-nops and fkeep-vars-live to Og only
>
> Bootstrapped and reg-tested on x86_64. ChangeLog entries and function
> header comments missing.
>
> Comments welcome.
>
Consider this gdb session in foo of pr54200-2.c (using -Os):
...
(gdb) n
26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
(gdb) p a
'a' has unknown type; cast it to its declared type
(gdb) n
main () at pr54200-2.c:34
34 return 0;
...
The problem is that the scope in which a is declared ends at .LBE7, and the
statement .loc for line 26 ends up attached to the ret insn:
...
.loc 1 24 11
addl %edx, %eax
.LVL1:
# DEBUG a => ax
.loc 1 26 7 is_stmt 1
.LBE7:
.loc 1 28 1 is_stmt 0
ret
.cfi_endproc
...
This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing
for O1 and higher) by adding a nop after the ".loc is_stmt 1" annotation:
...
.loc 1 24 11
addl %edx, %eax
.LVL1:
# DEBUG a => ax
.loc 1 26 7 is_stmt 1
+ nop
.LBE7:
.loc 1 28 1 is_stmt 0
ret
.cfi_endproc
...
and instead we have:
...
(gdb) n
26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
(gdb) p a
$1 = 6
(gdb) n
main () at pr54200-2.c:34
34 return 0;
...
The option should have the same effect as using a debugger with location views
capability.
There's a design principle in GCC that code generation and debug generation
are independent. This guarantees that if you're encountering a problem in an
application without debug info, you can recompile it with -g and be certain
that you can reproduce the same problem, and use the debug info to debug the
problem. This invariant is enforced by bootstrap-debug. The fdebug-nops
breaks this invariant, but we consider this acceptable since it is intended
for use in combination with -Og -g in the standard edit-compile-debug cycle,
where -g is assumed to be already present at the point of encountering a
problem.
PR debug/78685
[debug] Add fdebug-nops
---
gcc/common.opt | 4 +++
gcc/testsuite/gcc.dg/guality/pr54200-2.c | 37 +++++++++++++++++++++++++
gcc/var-tracking.c | 46 ++++++++++++++++++++++++++++++++
3 files changed, 87 insertions(+)
diff --git a/gcc/common.opt b/gcc/common.opt
index 4bf8de90331..984b351cd79 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1492,6 +1492,10 @@ Common Report Var(flag_hoist_adjacent_loads) Optimization
Enable hoisting adjacent loads to encourage generating conditional move
instructions.
+fdebug-nops
+Common Report Var(flag_debug_nops) Optimization
+Insert nops if that might improve debugging of optimized code.
+
fkeep-gc-roots-live
Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
; Always keep a pointer to a live memory block
diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
new file mode 100644
index 00000000000..e30e3c92b94
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
@@ -0,0 +1,37 @@
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
+/* { dg-options "-g -fdebug-nops -DMAIN" } */
+
+#include "prevent-optimization.h"
+
+int o ATTRIBUTE_USED;
+
+void
+bar (void)
+{
+ o = 2;
+}
+
+int __attribute__((noinline,noclone))
+foo (int z, int x, int b)
+{
+ if (x == 1)
+ {
+ bar ();
+ return z;
+ }
+ else
+ {
+ int a = (x + z) + b;
+ /* Add cast to prevent unsupported. */
+ return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
+ }
+}
+
+#ifdef MAIN
+int main ()
+{
+ foo (3, 2, 1);
+ return 0;
+}
+#endif
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 5537fa64085..1349454d5f2 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -10427,6 +10427,49 @@ vt_finalize (void)
vui_allocated = 0;
}
+static void
+insert_debug_nops (void)
+{
+ rtx_insn *debug_marker = NULL;
+
+ for (rtx_insn *insn = get_insns (); insn; insn = NEXT_INSN (insn))
+ {
+ bool use_p = INSN_P (insn) && GET_CODE (PATTERN (insn)) == USE;
+ bool clobber_p = INSN_P (insn) && GET_CODE (PATTERN (insn)) == CLOBBER;
+
+ if (!debug_marker)
+ {
+ if (DEBUG_MARKER_INSN_P (insn))
+ debug_marker = insn;
+ continue;
+ }
+
+ if (LABEL_P (insn) || BARRIER_P (insn) || DEBUG_MARKER_INSN_P (insn))
+ ;
+ else if (use_p || clobber_p || NOTE_P (insn) || DEBUG_INSN_P (insn)
+ || JUMP_TABLE_DATA_P (insn))
+ continue;
+ else if (INSN_P (insn))
+ {
+ unsigned int debug_marker_loc = INSN_LOCATION (debug_marker);
+ unsigned int insn_loc = INSN_LOCATION (insn);
+
+ if (LOCATION_FILE (insn_loc) == LOCATION_FILE (debug_marker_loc)
+ && LOCATION_LINE (insn_loc) == LOCATION_LINE (debug_marker_loc))
+ {
+ debug_marker = NULL;
+ continue;
+ }
+ }
+ else
+ gcc_unreachable ();
+
+ emit_insn_after_setloc (gen_nop (), debug_marker,
+ INSN_LOCATION (debug_marker));
+ debug_marker = DEBUG_MARKER_INSN_P (insn) ? insn : NULL;
+ }
+}
+
/* The entry point to variable tracking pass. */
static inline unsigned int
@@ -10434,6 +10477,9 @@ variable_tracking_main_1 (void)
{
bool success;
+ if (flag_debug_nops)
+ insert_debug_nops ();
+
/* We won't be called as a separate pass if flag_var_tracking is not
set, but final may call us to turn debug markers into notes. */
if ((!flag_var_tracking && MAY_HAVE_DEBUG_INSNS)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC 2/3, debug] Add fkeep-vars-live
2018-07-24 11:30 ` Tom de Vries
2018-07-24 11:35 ` [RFC 1/3, debug] Add fdebug-nops Tom de Vries
@ 2018-07-24 11:37 ` Tom de Vries
2018-07-24 11:46 ` Jakub Jelinek
2018-07-24 19:11 ` [RFC 2/3, " Alexandre Oliva
2018-07-24 11:38 ` [RFC 3/3, debug] Add fdebug-nops and fkeep-vars-live to Og only Tom de Vries
2 siblings, 2 replies; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 11:37 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek, Jim Wilson, aoliva
On Tue, Jul 24, 2018 at 01:30:30PM +0200, Tom de Vries wrote:
> On 07/16/2018 05:10 PM, Tom de Vries wrote:
> > On 07/16/2018 03:50 PM, Richard Biener wrote:
> >> On Mon, 16 Jul 2018, Tom de Vries wrote:
> >>> Any comments?
> >>
> >> Interesting idea. I wonder if that should be generalized
> >> to other places
> >
> > I kept the option name general, to allow for that.
> >
> > And indeed, this is a point-fix patch. I've been playing around with a
> > more generic patch that adds nops such that each is_stmt .loc is
> > associated with a unique insn, but that was embedded in an
> > fkeep-vars-live branch, so this patch is minimally addressing the first
> > problem I managed to reproduce on trunk.
> >
> >> and how we can avoid compare-debug failures
> >> (var-tracking usually doesn't change code-generation).
> >>
> >
>
> I'll post this patch series (the current state of my fkeep-vars-live
> branch) in reply to this email:
>
> 1 [debug] Add fdebug-nops
> 2 [debug] Add fkeep-vars-live
> 3 [debug] Add fdebug-nops and fkeep-vars-live to Og only
>
> Bootstrapped and reg-tested on x86_64. ChangeLog entries and function
> header comments missing.
>
> Comments welcome.
>
This patch adds fake uses of user variables at the point where they go out of
scope, to keep user variables inspectable throughout the application.
This approach will generate sub-optimal code: in some cases, the executable
code will go through efforts to keep a var alive, while var-tracking can
easily compute the value of the var from something else.
Also, the compiler treats the fake use as any other use, so it may keep an
expensive resource like a register occupied (if we could mark the use as a
cold use or some such, we could tell optimizers that we need the value, but
it's ok if getting the value is expensive, so it could be spilled instead of
occupying a register).
The current implementation is expected to increase register pressure, and
therefore spilling, but we'd still expect less memory accesses then with O0.
Another drawback is that the fake uses confuse the unitialized warning
analysis, so that is switched off for -fkeep-vars-live.
PR debug/78685
[debug] Add fkeep-vars-live
---
gcc/cfgexpand.c | 9 +++++++
gcc/common.opt | 4 +++
gcc/function.c | 5 ++--
gcc/gimplify.c | 44 +++++++++++++++++++++++++++-----
gcc/testsuite/gcc.dg/guality/pr54200-2.c | 3 +--
gcc/tree-cfg.c | 1 +
gcc/tree-sra.c | 7 +++++
gcc/tree-ssa-alias.c | 4 +++
gcc/tree-ssa-structalias.c | 3 ++-
gcc/tree-ssa-uninit.c | 2 +-
10 files changed, 70 insertions(+), 12 deletions(-)
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index d6e3c382085..eb9e36a8c5b 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -3533,6 +3533,13 @@ expand_clobber (tree lhs)
}
}
+static void
+expand_use (tree rhs)
+{
+ rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
+ emit_use (target);
+}
+
/* A subroutine of expand_gimple_stmt, expanding one gimple statement
STMT that doesn't require special handling for outgoing edges. That
is no tailcalls and no GIMPLE_COND. */
@@ -3632,6 +3639,8 @@ expand_gimple_stmt_1 (gimple *stmt)
/* This is a clobber to mark the going out of scope for
this LHS. */
expand_clobber (lhs);
+ else if (TREE_CLOBBER_P (lhs))
+ expand_use (rhs);
else
expand_assignment (lhs, rhs,
gimple_assign_nontemporal_move_p (
diff --git a/gcc/common.opt b/gcc/common.opt
index 984b351cd79..a29e320ba45 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1496,6 +1496,10 @@ fdebug-nops
Common Report Var(flag_debug_nops) Optimization
Insert nops if that might improve debugging of optimized code.
+fkeep-vars-live
+Common Report Var(flag_keep_vars_live) Optimization
+Add artificial uses of local vars at end of scope.
+
fkeep-gc-roots-live
Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
; Always keep a pointer to a live memory block
diff --git a/gcc/function.c b/gcc/function.c
index dee303cdbdd..6367d282db3 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -1964,11 +1964,12 @@ instantiate_virtual_regs (void)
{
/* These patterns in the instruction stream can never be recognized.
Fortunately, they shouldn't contain virtual registers either. */
- if (GET_CODE (PATTERN (insn)) == USE
- || GET_CODE (PATTERN (insn)) == CLOBBER
+ if (GET_CODE (PATTERN (insn)) == CLOBBER
|| GET_CODE (PATTERN (insn)) == ASM_INPUT
|| DEBUG_MARKER_INSN_P (insn))
continue;
+ else if (GET_CODE (PATTERN (insn)) == USE)
+ instantiate_virtual_regs_in_rtx (&PATTERN (insn));
else if (DEBUG_BIND_INSN_P (insn))
instantiate_virtual_regs_in_rtx (INSN_VAR_LOCATION_PTR (insn));
else
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 4a109aee27a..1ce6ef648c6 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1264,6 +1264,23 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p
}
}
+static gimple_seq
+gimple_build_uses (tree vars)
+{
+ gimple_seq seq = NULL;
+
+ for (tree var = vars; var; var = DECL_CHAIN (var))
+ {
+ if (DECL_ARTIFICIAL (var))
+ continue;
+
+ gimple *stmt = gimple_build_assign (build_clobber (TREE_TYPE (var)), var);
+ gimple_seq_add_stmt (&seq, stmt);
+ }
+
+ return seq;
+}
+
/* Gimplify a BIND_EXPR. Just voidify and recurse. */
static enum gimplify_status
@@ -1363,7 +1380,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
gimplify_seq_add_stmt (&cleanup, stack_restore);
}
- /* Add clobbers for all variables that go out of scope. */
+ /* Add clobbers/uses for all variables that go out of scope. */
for (t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
{
if (VAR_P (t)
@@ -1376,14 +1393,17 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
/* Only care for variables that have to be in memory. Others
will be rewritten into SSA names, hence moved to the
top-level. */
- && !is_gimple_reg (t)
+ && (flag_keep_vars_live || !is_gimple_reg (t))
&& flag_stack_reuse != SR_NONE)
{
tree clobber = build_clobber (TREE_TYPE (t));
- gimple *clobber_stmt;
- clobber_stmt = gimple_build_assign (t, clobber);
- gimple_set_location (clobber_stmt, end_locus);
- gimplify_seq_add_stmt (&cleanup, clobber_stmt);
+ gimple *stmt;
+ if (is_gimple_reg (t))
+ stmt = gimple_build_assign (clobber, t);
+ else
+ stmt = gimple_build_assign (t, clobber);
+ gimple_set_location (stmt, end_locus);
+ gimplify_seq_add_stmt (&cleanup, stmt);
}
if (flag_openacc && oacc_declare_returns != NULL)
@@ -12763,6 +12783,10 @@ gimplify_body (tree fndecl, bool do_parms)
gimple *outer_stmt;
gbind *outer_bind;
+ gimple_seq cleanup = NULL;
+ if (flag_keep_vars_live)
+ cleanup = gimple_build_uses (DECL_ARGUMENTS (fndecl));
+
timevar_push (TV_TREE_GIMPLIFY);
init_tree_ssa (cfun);
@@ -12798,6 +12822,14 @@ gimplify_body (tree fndecl, bool do_parms)
/* Gimplify the function's body. */
seq = NULL;
gimplify_stmt (&DECL_SAVED_TREE (fndecl), &seq);
+
+ if (cleanup)
+ {
+ gtry *gs = gimple_build_try (seq, cleanup, GIMPLE_TRY_FINALLY);
+ seq = NULL;
+ gimplify_seq_add_stmt (&seq, gs);
+ }
+
outer_stmt = gimple_seq_first_stmt (seq);
if (!outer_stmt)
{
diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
index e30e3c92b94..646790af65a 100644
--- a/gcc/testsuite/gcc.dg/guality/pr54200-2.c
+++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
@@ -1,6 +1,5 @@
/* { dg-do run } */
-/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
-/* { dg-options "-g -fdebug-nops -DMAIN" } */
+/* { dg-options "-g -fdebug-nops -fkeep-vars-live -DMAIN" } */
#include "prevent-optimization.h"
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 14d66b7a728..d3a4465fe25 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -4484,6 +4484,7 @@ verify_gimple_assign_single (gassign *stmt)
case PARM_DECL:
if (!is_gimple_reg (lhs)
&& !is_gimple_reg (rhs1)
+ && !TREE_CLOBBER_P (lhs)
&& is_gimple_reg_type (TREE_TYPE (lhs)))
{
error ("invalid rhs for gimple memory store");
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 3e30f6bc3d4..f6488b90378 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -3536,6 +3536,13 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
sra_stats.exprs++;
}
+ if (modify_this_stmt && TREE_CLOBBER_P (gimple_assign_lhs (stmt)))
+ {
+ gimple_assign_set_rhs1 (stmt, rhs);
+ gimple_assign_set_lhs (stmt, build_clobber (TREE_TYPE (rhs)));
+ return SRA_AM_MODIFIED;
+ }
+
if (modify_this_stmt)
{
if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
index 7b25778307f..007e6f84c2b 100644
--- a/gcc/tree-ssa-alias.c
+++ b/gcc/tree-ssa-alias.c
@@ -1368,6 +1368,10 @@ refs_may_alias_p_1 (ao_ref *ref1, ao_ref *ref2, bool tbaa_p)
poly_int64 max_size1 = -1, max_size2 = -1;
bool var1_p, var2_p, ind1_p, ind2_p;
+ if ((ref1->ref && TREE_CLOBBER_P (ref1->ref)) ||
+ (ref2->ref && TREE_CLOBBER_P (ref2->ref)))
+ return false;
+
gcc_checking_assert ((!ref1->ref
|| TREE_CODE (ref1->ref) == SSA_NAME
|| DECL_P (ref1->ref)
diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index fd24f84fb14..d83579a09c5 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -4883,7 +4883,8 @@ find_func_aliases (struct function *fn, gimple *origt)
tree lhsop = gimple_assign_lhs (t);
tree rhsop = (gimple_num_ops (t) == 2) ? gimple_assign_rhs1 (t) : NULL;
- if (rhsop && TREE_CLOBBER_P (rhsop))
+ if ((rhsop && TREE_CLOBBER_P (rhsop))
+ || (lhsop && TREE_CLOBBER_P (lhsop)))
/* Ignore clobbers, they don't actually store anything into
the LHS. */
;
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 8ccbc85970a..77f090bfa80 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -2628,7 +2628,7 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
static bool
gate_warn_uninitialized (void)
{
- return warn_uninitialized || warn_maybe_uninitialized;
+ return (warn_uninitialized || warn_maybe_uninitialized) && !flag_keep_vars_live;
}
namespace {
^ permalink raw reply [flat|nested] 23+ messages in thread
* [RFC 3/3, debug] Add fdebug-nops and fkeep-vars-live to Og only
2018-07-24 11:30 ` Tom de Vries
2018-07-24 11:35 ` [RFC 1/3, debug] Add fdebug-nops Tom de Vries
2018-07-24 11:37 ` [RFC 2/3, debug] Add fkeep-vars-live Tom de Vries
@ 2018-07-24 11:38 ` Tom de Vries
2 siblings, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 11:38 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek, Jim Wilson, aoliva
On Tue, Jul 24, 2018 at 01:30:30PM +0200, Tom de Vries wrote:
> On 07/16/2018 05:10 PM, Tom de Vries wrote:
> > On 07/16/2018 03:50 PM, Richard Biener wrote:
> >> On Mon, 16 Jul 2018, Tom de Vries wrote:
> >>> Any comments?
> >>
> >> Interesting idea. I wonder if that should be generalized
> >> to other places
> >
> > I kept the option name general, to allow for that.
> >
> > And indeed, this is a point-fix patch. I've been playing around with a
> > more generic patch that adds nops such that each is_stmt .loc is
> > associated with a unique insn, but that was embedded in an
> > fkeep-vars-live branch, so this patch is minimally addressing the first
> > problem I managed to reproduce on trunk.
> >
> >> and how we can avoid compare-debug failures
> >> (var-tracking usually doesn't change code-generation).
> >>
> >
>
> I'll post this patch series (the current state of my fkeep-vars-live
> branch) in reply to this email:
>
> 1 [debug] Add fdebug-nops
> 2 [debug] Add fkeep-vars-live
> 3 [debug] Add fdebug-nops and fkeep-vars-live to Og only
>
> Bootstrapped and reg-tested on x86_64. ChangeLog entries and function
> header comments missing.
>
> Comments welcome.
>
This switches on fdebug-nops and fkeep-vars-live by default for Og only, in
order to excercise the new switches in combination with the optimization
option they're intended to be used with.
Resulting fixes:
...
FAIL: gcc.dg/guality/pr54200.c -Og -DPREVENT_OPTIMIZATION line . z == 3
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+4 a[0] == 1
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+3 a[1] == 2
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+2 a[2] == 3
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+1 *p == 3
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line . *q == 2
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+4 a[0] == 1
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+3 a[1] == 2
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+2 a[2] == 13
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+1 *p == 13
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line . *q == 2
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+4 a[0] == 1
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+3 a[1] == 12
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+2 a[2] == 13
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+1 *p == 13
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line . *q == 12
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+3 a[1] == 5
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+2 a[2] == 6
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+1 *p == 6
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line . *q == 5
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+3 a[1] == 5
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+2 a[2] == 26
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+1 *p == 26
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line . *q == 5
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+7 a[1] == 25
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+6 a[2] == 26
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+5 *p == 26
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+4 p[-1] == 25
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line .+1 q[1] == 26
FAIL: gcc.dg/guality/pr54970.c -Og -DPREVENT_OPTIMIZATION line . *q == 25
...
The intended nature of Og is: offering a reasonable level of optimization
while maintaining fast compilation and a good debugging experience.
Todo: assess compile time and runtime cost of adding adding fdebug-nops and
fkeep-vars-live to Og, to see if nature of Og has been changed.
PR debug/78685
[debug] Add fdebug-nops and fkeep-vars-live to Og only
---
gcc/common/common-target.h | 1 +
gcc/opts.c | 6 ++++++
gcc/testsuite/gcc.dg/pr84614.c | 2 +-
3 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/gcc/common/common-target.h b/gcc/common/common-target.h
index 9b796c8f085..17e8950a027 100644
--- a/gcc/common/common-target.h
+++ b/gcc/common/common-target.h
@@ -34,6 +34,7 @@ enum opt_levels
OPT_LEVELS_1_PLUS, /* -O1 and above, including -Os and -Og. */
OPT_LEVELS_1_PLUS_SPEED_ONLY, /* -O1 and above, but not -Os or -Og. */
OPT_LEVELS_1_PLUS_NOT_DEBUG, /* -O1 and above, but not -Og. */
+ OPT_LEVELS_1_DEBUG, /* -Og. */
OPT_LEVELS_2_PLUS, /* -O2 and above, including -Os. */
OPT_LEVELS_2_PLUS_SPEED_ONLY, /* -O2 and above, but not -Os or -Og. */
OPT_LEVELS_3_PLUS, /* -O3 and above. */
diff --git a/gcc/opts.c b/gcc/opts.c
index 17d91988ada..e8142eb6920 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -370,6 +370,10 @@ maybe_default_option (struct gcc_options *opts,
enabled = (level >= 1 && !debug);
break;
+ case OPT_LEVELS_1_DEBUG:
+ enabled = (level == 1 && debug);
+ break;
+
case OPT_LEVELS_2_PLUS:
enabled = (level >= 2);
break;
@@ -477,6 +481,8 @@ static const struct default_options default_options_table[] =
{ OPT_LEVELS_1_PLUS_NOT_DEBUG, OPT_fssa_phiopt, NULL, 1 },
{ OPT_LEVELS_1_PLUS, OPT_ftree_builtin_call_dce, NULL, 1 },
{ OPT_LEVELS_1_PLUS, OPT_fomit_frame_pointer, NULL, 1 },
+ { OPT_LEVELS_1_DEBUG, OPT_fdebug_nops, NULL, 1 },
+ { OPT_LEVELS_1_DEBUG, OPT_fkeep_vars_live, NULL, 1 },
/* -O2 optimizations. */
{ OPT_LEVELS_2_PLUS, OPT_finline_small_functions, NULL, 1 },
diff --git a/gcc/testsuite/gcc.dg/pr84614.c b/gcc/testsuite/gcc.dg/pr84614.c
index 98af26ba4e5..b7a5b9c1c84 100644
--- a/gcc/testsuite/gcc.dg/pr84614.c
+++ b/gcc/testsuite/gcc.dg/pr84614.c
@@ -1,6 +1,6 @@
/* PR target/84614 */
/* { dg-do run { target int128 } } */
-/* { dg-options "-Og -fno-split-wide-types -fno-tree-coalesce-vars -g --param=max-combine-insns=3 -fcompare-debug" } */
+/* { dg-options "-Og -fno-split-wide-types -fno-tree-coalesce-vars -g --param=max-combine-insns=3 -fcompare-debug -fno-debug-nops" } */
unsigned __int128 a;
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 2/3, debug] Add fkeep-vars-live
2018-07-24 11:37 ` [RFC 2/3, debug] Add fkeep-vars-live Tom de Vries
@ 2018-07-24 11:46 ` Jakub Jelinek
2018-07-24 12:34 ` Tom de Vries
2018-07-24 19:11 ` [RFC 2/3, " Alexandre Oliva
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Jelinek @ 2018-07-24 11:46 UTC (permalink / raw)
To: Tom de Vries; +Cc: Richard Biener, gcc-patches, Jim Wilson, aoliva
On Tue, Jul 24, 2018 at 01:37:32PM +0200, Tom de Vries wrote:
> Another drawback is that the fake uses confuse the unitialized warning
> analysis, so that is switched off for -fkeep-vars-live.
Is that really needed? I.e. can't you for the purpose of uninitialized
warning analysis ignore the clobber = var uses?
Is the -fkeep-vars-live option -fcompare-debug friendly?
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -3533,6 +3533,13 @@ expand_clobber (tree lhs)
> }
> }
>
> +static void
> +expand_use (tree rhs)
> +{
> + rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> + emit_use (target);
> +}
Missing function comment.
> +fkeep-vars-live
> +Common Report Var(flag_keep_vars_live) Optimization
> +Add artificial uses of local vars at end of scope.
at the end of scope?
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1264,6 +1264,23 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p
> }
> }
>
> +static gimple_seq
> +gimple_build_uses (tree vars)
Missing function comment.
> --- a/gcc/tree-ssa-alias.c
> +++ b/gcc/tree-ssa-alias.c
> @@ -1368,6 +1368,10 @@ refs_may_alias_p_1 (ao_ref *ref1, ao_ref *ref2, bool tbaa_p)
> poly_int64 max_size1 = -1, max_size2 = -1;
> bool var1_p, var2_p, ind1_p, ind2_p;
>
> + if ((ref1->ref && TREE_CLOBBER_P (ref1->ref)) ||
|| should go on the next line.
> + (ref2->ref && TREE_CLOBBER_P (ref2->ref)))
> + return false;
> +
> gcc_checking_assert ((!ref1->ref
> || TREE_CODE (ref1->ref) == SSA_NAME
> || DECL_P (ref1->ref)
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -2628,7 +2628,7 @@ warn_uninitialized_phi (gphi *phi, vec<gphi *> *worklist,
> static bool
> gate_warn_uninitialized (void)
> {
> - return warn_uninitialized || warn_maybe_uninitialized;
> + return (warn_uninitialized || warn_maybe_uninitialized) && !flag_keep_vars_live;
> }
See above.
Jakub
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 2/3, debug] Add fkeep-vars-live
2018-07-24 11:46 ` Jakub Jelinek
@ 2018-07-24 12:34 ` Tom de Vries
2018-07-24 15:03 ` [PATCH, " Tom de Vries
0 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 12:34 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, gcc-patches, Jim Wilson, aoliva
On 07/24/2018 01:46 PM, Jakub Jelinek wrote:
> On Tue, Jul 24, 2018 at 01:37:32PM +0200, Tom de Vries wrote:
>> Another drawback is that the fake uses confuse the unitialized warning
>> analysis, so that is switched off for -fkeep-vars-live.
>
> Is that really needed? I.e. can't you for the purpose of uninitialized
> warning analysis ignore the clobber = var uses?
>
This seems to work on the test-case that failed during testing
(g++.dg/uninit-pred-4.C):
...
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 77f090bfa80..953db9ed02d 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr,
tree var,
if (is_gimple_assign (context)
&& gimple_assign_rhs_code (context) == COMPLEX_EXPR)
return;
+ if (gimple_assign_single_p (context)
+ && TREE_CLOBBER_P (gimple_assign_lhs (context)))
+ return;
if (!has_undefined_value_p (t))
return;
...
But I don't know the pass well enough to know whether this is a
sufficient fix.
> Is the -fkeep-vars-live option -fcompare-debug friendly?
>
I think so, there's no reference to debug flags or instructions.
>> --- a/gcc/cfgexpand.c
>> +++ b/gcc/cfgexpand.c
>> @@ -3533,6 +3533,13 @@ expand_clobber (tree lhs)
>> }
>> }
>>
>> +static void
>> +expand_use (tree rhs)
>> +{
>> + rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
>> + emit_use (target);
>> +}
>
> Missing function comment.
>
>> +fkeep-vars-live
>> +Common Report Var(flag_keep_vars_live) Optimization
>> +Add artificial uses of local vars at end of scope.
>
> at the end of scope?
Is this better?
+Add artificial use for each local variable at the end of the
declaration scope
Thanks,
- Tom
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH, debug] Add fdebug-nops
2018-07-24 11:35 ` [RFC 1/3, debug] Add fdebug-nops Tom de Vries
@ 2018-07-24 14:59 ` Tom de Vries
2018-07-24 19:07 ` [RFC 1/3, " Alexandre Oliva
1 sibling, 0 replies; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 14:59 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, Jakub Jelinek, Jim Wilson, aoliva
On Tue, Jul 24, 2018 at 01:35:14PM +0200, Tom de Vries wrote:
> On Tue, Jul 24, 2018 at 01:30:30PM +0200, Tom de Vries wrote:
> > On 07/16/2018 05:10 PM, Tom de Vries wrote:
> > > On 07/16/2018 03:50 PM, Richard Biener wrote:
> > >> On Mon, 16 Jul 2018, Tom de Vries wrote:
> > >>> Any comments?
> > >>
> > >> Interesting idea. I wonder if that should be generalized
> > >> to other places
> > >
> > > I kept the option name general, to allow for that.
> > >
> > > And indeed, this is a point-fix patch. I've been playing around with a
> > > more generic patch that adds nops such that each is_stmt .loc is
> > > associated with a unique insn, but that was embedded in an
> > > fkeep-vars-live branch, so this patch is minimally addressing the first
> > > problem I managed to reproduce on trunk.
> > >
> > >> and how we can avoid compare-debug failures
> > >> (var-tracking usually doesn't change code-generation).
> > >>
> > >
> >
> > I'll post this patch series (the current state of my fkeep-vars-live
> > branch) in reply to this email:
> >
> > 1 [debug] Add fdebug-nops
> > 2 [debug] Add fkeep-vars-live
> > 3 [debug] Add fdebug-nops and fkeep-vars-live to Og only
> >
> > Bootstrapped and reg-tested on x86_64. ChangeLog entries and function
> > header comments missing.
> >
> > Comments welcome.
> >
>
And here's a version that is fcompare-debug friendly.
OK for trunk?
Thanks,
- Tom
[debug] Add fdebug-nops
Consider this gdb session in foo of pr54200-2.c (using -Os):
...
(gdb) n
26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
(gdb) p a
'a' has unknown type; cast it to its declared type
(gdb) n
main () at pr54200-2.c:34
34 return 0;
...
The problem is that the scope in which a is declared ends at .LBE7, and the
statement .loc for line 26 ends up attached to the ret insn:
...
.loc 1 24 11
addl %edx, %eax
.LVL1:
# DEBUG a => ax
.loc 1 26 7 is_stmt 1
.LBE7:
.loc 1 28 1 is_stmt 0
ret
.cfi_endproc
...
This patch fixes the problem (for Og and Os, the 'DEBUG a => ax' is missing
for O1 and higher) by adding a nop after the ".loc is_stmt 1" annotation:
...
.loc 1 24 11
addl %edx, %eax
.LVL1:
# DEBUG a => ax
.loc 1 26 7 is_stmt 1
+ nop
.LBE7:
.loc 1 28 1 is_stmt 0
ret
.cfi_endproc
...
and instead we have:
...
(gdb) n
26 return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
(gdb) p a
$1 = 6
(gdb) n
main () at pr54200-2.c:34
34 return 0;
...
The option should have the same effect as using a debugger with location views
capability.
2018-07-24 Tom de Vries <tdevries@suse.de>
PR debug/78685
* common.opt (fdebug-nops): New option.
* passes.def (pass_late_compilation): Add pass_debug_nops.
* rtl.h (MAY_HAVE_DEBUG_MARKER_INSNS): Add flag_debug_nops.
* timevar.def (TV_VAR_DEBUG_NOPS): New timevar.
* tree-pass.h (make_pass_debug_nops): Declare.
* tree.h (MAY_HAVE_DEBUG_MARKER_STMTS): Add flag_debug_nops.
* var-tracking.c (insert_debug_nops): New function.
(class pass_debug_nops): New pass.
(make_pass_debug_nops): New function.
* gcc.dg/guality/pr54200-2.c: New test.
---
gcc/common.opt | 4 ++
gcc/passes.def | 1 +
gcc/rtl.h | 2 +-
gcc/testsuite/gcc.dg/guality/pr54200-2.c | 37 +++++++++++++
gcc/timevar.def | 1 +
gcc/tree-pass.h | 1 +
gcc/tree.h | 2 +-
gcc/var-tracking.c | 91 ++++++++++++++++++++++++++++++++
8 files changed, 137 insertions(+), 2 deletions(-)
diff --git a/gcc/common.opt b/gcc/common.opt
index 4bf8de90331..984b351cd79 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1492,6 +1492,10 @@ Common Report Var(flag_hoist_adjacent_loads) Optimization
Enable hoisting adjacent loads to encourage generating conditional move
instructions.
+fdebug-nops
+Common Report Var(flag_debug_nops) Optimization
+Insert nops if that might improve debugging of optimized code.
+
fkeep-gc-roots-live
Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
; Always keep a pointer to a live memory block
diff --git a/gcc/passes.def b/gcc/passes.def
index 2a8fbc2efbe..e97c4f9533a 100644
--- a/gcc/passes.def
+++ b/gcc/passes.def
@@ -487,6 +487,7 @@ along with GCC; see the file COPYING3. If not see
NEXT_PASS (pass_late_compilation);
PUSH_INSERT_PASSES_WITHIN (pass_late_compilation)
NEXT_PASS (pass_compute_alignments);
+ NEXT_PASS (pass_debug_nops);
NEXT_PASS (pass_variable_tracking);
NEXT_PASS (pass_free_cfg);
NEXT_PASS (pass_machine_reorg);
diff --git a/gcc/rtl.h b/gcc/rtl.h
index 565ce3abbe4..29a2e19cb6e 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -843,7 +843,7 @@ struct GTY(()) rtvec_def {
#define NONDEBUG_INSN_P(X) (INSN_P (X) && !DEBUG_INSN_P (X))
/* Nonzero if DEBUG_MARKER_INSN_P may possibly hold. */
-#define MAY_HAVE_DEBUG_MARKER_INSNS debug_nonbind_markers_p
+#define MAY_HAVE_DEBUG_MARKER_INSNS (debug_nonbind_markers_p || flag_debug_nops)
/* Nonzero if DEBUG_BIND_INSN_P may possibly hold. */
#define MAY_HAVE_DEBUG_BIND_INSNS flag_var_tracking_assignments
/* Nonzero if DEBUG_INSN_P may possibly hold. */
diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
new file mode 100644
index 00000000000..e30e3c92b94
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
@@ -0,0 +1,37 @@
+/* { dg-do run } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
+/* { dg-options "-g -fdebug-nops -DMAIN" } */
+
+#include "prevent-optimization.h"
+
+int o ATTRIBUTE_USED;
+
+void
+bar (void)
+{
+ o = 2;
+}
+
+int __attribute__((noinline,noclone))
+foo (int z, int x, int b)
+{
+ if (x == 1)
+ {
+ bar ();
+ return z;
+ }
+ else
+ {
+ int a = (x + z) + b;
+ /* Add cast to prevent unsupported. */
+ return a; /* { dg-final { gdb-test . "(int)a" "6" } } */
+ }
+}
+
+#ifdef MAIN
+int main ()
+{
+ foo (3, 2, 1);
+ return 0;
+}
+#endif
diff --git a/gcc/timevar.def b/gcc/timevar.def
index 91221ae5b0e..10216c669de 100644
--- a/gcc/timevar.def
+++ b/gcc/timevar.def
@@ -286,6 +286,7 @@ DEFTIMEVAR (TV_SYMOUT , "symout")
DEFTIMEVAR (TV_VAR_TRACKING , "variable tracking")
DEFTIMEVAR (TV_VAR_TRACKING_DATAFLOW , "var-tracking dataflow")
DEFTIMEVAR (TV_VAR_TRACKING_EMIT , "var-tracking emit")
+DEFTIMEVAR (TV_VAR_DEBUG_NOPS , "debug nops emit")
DEFTIMEVAR (TV_TREE_IFCOMBINE , "tree if-combine")
DEFTIMEVAR (TV_TREE_UNINIT , "uninit var analysis")
DEFTIMEVAR (TV_PLUGIN_INIT , "plugin initialization")
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index af15adc8e0c..9cce0e1a7cd 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -603,6 +603,7 @@ extern rtl_opt_pass *make_pass_stack_regs_run (gcc::context *ctxt);
extern rtl_opt_pass *make_pass_df_finish (gcc::context *ctxt);
extern rtl_opt_pass *make_pass_compute_alignments (gcc::context *ctxt);
extern rtl_opt_pass *make_pass_duplicate_computed_gotos (gcc::context *ctxt);
+extern rtl_opt_pass *make_pass_debug_nops (gcc::context *ctxt);
extern rtl_opt_pass *make_pass_variable_tracking (gcc::context *ctxt);
extern rtl_opt_pass *make_pass_free_cfg (gcc::context *ctxt);
extern rtl_opt_pass *make_pass_machine_reorg (gcc::context *ctxt);
diff --git a/gcc/tree.h b/gcc/tree.h
index 70ac78130c0..67985617863 100644
--- a/gcc/tree.h
+++ b/gcc/tree.h
@@ -1126,7 +1126,7 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
((int)TREE_INT_CST_LOW (VL_EXP_CHECK (NODE)->exp.operands[0]))
/* Nonzero if gimple_debug_nonbind_marker_p() may possibly hold. */
-#define MAY_HAVE_DEBUG_MARKER_STMTS debug_nonbind_markers_p
+#define MAY_HAVE_DEBUG_MARKER_STMTS (debug_nonbind_markers_p || flag_debug_nops)
/* Nonzero if gimple_debug_bind_p() (and thus
gimple_debug_source_bind_p()) may possibly hold. */
#define MAY_HAVE_DEBUG_BIND_STMTS flag_var_tracking_assignments
diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
index 5537fa64085..04e878610e9 100644
--- a/gcc/var-tracking.c
+++ b/gcc/var-tracking.c
@@ -10559,3 +10559,94 @@ make_pass_variable_tracking (gcc::context *ctxt)
{
return new pass_variable_tracking (ctxt);
}
+
+/* Insert nop after debug marker insn, if that insn is not followed by a real
+ insn on the same line. */
+
+static void
+insert_debug_nops (void)
+{
+ rtx_insn *debug_marker = NULL;
+
+ for (rtx_insn *insn = get_insns (); insn; insn = NEXT_INSN (insn))
+ {
+ bool use_p = INSN_P (insn) && GET_CODE (PATTERN (insn)) == USE;
+ bool clobber_p = INSN_P (insn) && GET_CODE (PATTERN (insn)) == CLOBBER;
+
+ if (!debug_marker)
+ {
+ if (DEBUG_MARKER_INSN_P (insn))
+ debug_marker = insn;
+ continue;
+ }
+
+ if (LABEL_P (insn) || BARRIER_P (insn) || DEBUG_MARKER_INSN_P (insn))
+ ;
+ else if (use_p || clobber_p || NOTE_P (insn) || DEBUG_INSN_P (insn)
+ || JUMP_TABLE_DATA_P (insn))
+ continue;
+ else if (INSN_P (insn))
+ {
+ unsigned int debug_marker_loc = INSN_LOCATION (debug_marker);
+ unsigned int insn_loc = INSN_LOCATION (insn);
+
+ if (LOCATION_FILE (insn_loc) == LOCATION_FILE (debug_marker_loc)
+ && LOCATION_LINE (insn_loc) == LOCATION_LINE (debug_marker_loc))
+ {
+ debug_marker = NULL;
+ continue;
+ }
+ }
+ else
+ gcc_unreachable ();
+
+ emit_insn_after_setloc (gen_nop (), debug_marker,
+ INSN_LOCATION (debug_marker));
+ debug_marker = DEBUG_MARKER_INSN_P (insn) ? insn : NULL;
+ }
+}
+
+namespace {
+
+const pass_data pass_data_debug_nops =
+{
+ RTL_PASS, /* type */
+ "debugnops", /* name */
+ OPTGROUP_NONE, /* optinfo_flags */
+ TV_VAR_DEBUG_NOPS, /* tv_id */
+ 0, /* properties_required */
+ 0, /* properties_provided */
+ 0, /* properties_destroyed */
+ 0, /* todo_flags_start */
+ 0, /* todo_flags_finish */
+};
+
+class pass_debug_nops : public rtl_opt_pass
+{
+public:
+ pass_debug_nops (gcc::context *ctxt)
+ : rtl_opt_pass (pass_data_debug_nops, ctxt)
+ {}
+
+ /* opt_pass methods: */
+ virtual bool gate (function *)
+ {
+ return flag_debug_nops;
+ }
+
+ virtual unsigned int execute (function *)
+ {
+ insert_debug_nops ();
+ /* If !debug_nonbind_markers_p, we can drop those insn here. */
+ return 0;
+ }
+
+}; // class pass_debug_nops
+
+} // anon namespace
+
+rtl_opt_pass *
+make_pass_debug_nops (gcc::context *ctxt)
+{
+ return new pass_debug_nops (ctxt);
+}
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH, debug] Add fkeep-vars-live
2018-07-24 12:34 ` Tom de Vries
@ 2018-07-24 15:03 ` Tom de Vries
2018-07-25 11:06 ` Jakub Jelinek
2018-07-25 11:41 ` Richard Biener
0 siblings, 2 replies; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 15:03 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, gcc-patches, Jim Wilson, aoliva
On Tue, Jul 24, 2018 at 02:34:26PM +0200, Tom de Vries wrote:
> On 07/24/2018 01:46 PM, Jakub Jelinek wrote:
> > On Tue, Jul 24, 2018 at 01:37:32PM +0200, Tom de Vries wrote:
> >> Another drawback is that the fake uses confuse the unitialized warning
> >> analysis, so that is switched off for -fkeep-vars-live.
> >
> > Is that really needed? I.e. can't you for the purpose of uninitialized
> > warning analysis ignore the clobber = var uses?
> >
>
> This seems to work on the test-case that failed during testing
> (g++.dg/uninit-pred-4.C):
> ...
> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> index 77f090bfa80..953db9ed02d 100644
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr,
> tree var,
> if (is_gimple_assign (context)
> && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
> return;
> + if (gimple_assign_single_p (context)
> + && TREE_CLOBBER_P (gimple_assign_lhs (context)))
> + return;
> if (!has_undefined_value_p (t))
> return;
> ...
> But I don't know the pass well enough to know whether this is a
> sufficient fix.
>
Updated and re-tested patch.
> +Add artificial use for each local variable at the end of the
> declaration scope
Is this a better option description?
OK for trunk?
Thanks,
- Tom
[debug] Add fkeep-vars-live
This patch adds fake uses of user variables at the point where they go out of
scope, to keep user variables inspectable throughout the application.
This approach will generate sub-optimal code: in some cases, the executable
code will go through efforts to keep a var alive, while var-tracking can
easily compute the value of the var from something else.
Also, the compiler treats the fake use as any other use, so it may keep an
expensive resource like a register occupied (if we could mark the use as a
cold use or some such, we could tell optimizers that we need the value, but
it's ok if getting the value is expensive, so it could be spilled instead of
occupying a register).
The current implementation is expected to increase register pressure, and
therefore spilling, but we'd still expect less memory accesses then with O0.
2018-07-24 Tom de Vries <tdevries@suse.de>
PR debug/78685
* cfgexpand.c (expand_use): New function.
(expand_gimple_stmt_1): Handle TREE_CLOBBER_P as lhs of assignment.
* common.opt (fkeep-vars-live): New option.
* function.c (instantiate_virtual_regs): Instantiate in USEs as well.
* gimplify.c (gimple_build_uses): New function.
(gimplify_bind_expr): Build clobber uses for variables that don't have
to be in memory.
(gimplify_body): Build clobber uses for arguments.
* tree-cfg.c (verify_gimple_assign_single): Handle TREE_CLOBBER_P as lhs
of assignment.
* tree-sra.c (sra_modify_assign): Same.
* tree-ssa-alias.c (refs_may_alias_p_1): Same.
* tree-ssa-structalias.c (find_func_aliases): Same.
* tree-ssa-uninit.c (warn_uninit): Same.
* gcc.dg/guality/pr54200-2.c: Update.
---
gcc/cfgexpand.c | 11 ++++++++
gcc/common.opt | 4 +++
gcc/function.c | 5 ++--
gcc/gimplify.c | 46 +++++++++++++++++++++++++++-----
gcc/testsuite/gcc.dg/guality/pr54200-2.c | 3 +--
gcc/tree-cfg.c | 1 +
gcc/tree-sra.c | 7 +++++
gcc/tree-ssa-alias.c | 4 +++
gcc/tree-ssa-structalias.c | 3 ++-
gcc/tree-ssa-uninit.c | 3 +++
10 files changed, 76 insertions(+), 11 deletions(-)
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index d6e3c382085..e28e8ceec75 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -3533,6 +3533,15 @@ expand_clobber (tree lhs)
}
}
+/* Expand a use of RHS. */
+
+static void
+expand_use (tree rhs)
+{
+ rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
+ emit_use (target);
+}
+
/* A subroutine of expand_gimple_stmt, expanding one gimple statement
STMT that doesn't require special handling for outgoing edges. That
is no tailcalls and no GIMPLE_COND. */
@@ -3632,6 +3641,8 @@ expand_gimple_stmt_1 (gimple *stmt)
/* This is a clobber to mark the going out of scope for
this LHS. */
expand_clobber (lhs);
+ else if (TREE_CLOBBER_P (lhs))
+ expand_use (rhs);
else
expand_assignment (lhs, rhs,
gimple_assign_nontemporal_move_p (
diff --git a/gcc/common.opt b/gcc/common.opt
index 984b351cd79..a29e320ba45 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -1496,6 +1496,10 @@ fdebug-nops
Common Report Var(flag_debug_nops) Optimization
Insert nops if that might improve debugging of optimized code.
+fkeep-vars-live
+Common Report Var(flag_keep_vars_live) Optimization
+Add artificial uses of local vars at end of scope.
+
fkeep-gc-roots-live
Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
; Always keep a pointer to a live memory block
diff --git a/gcc/function.c b/gcc/function.c
index dee303cdbdd..b6aa1eb60d1 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -1964,11 +1964,12 @@ instantiate_virtual_regs (void)
{
/* These patterns in the instruction stream can never be recognized.
Fortunately, they shouldn't contain virtual registers either. */
- if (GET_CODE (PATTERN (insn)) == USE
- || GET_CODE (PATTERN (insn)) == CLOBBER
+ if (GET_CODE (PATTERN (insn)) == CLOBBER
|| GET_CODE (PATTERN (insn)) == ASM_INPUT
|| DEBUG_MARKER_INSN_P (insn))
continue;
+ else if (GET_CODE (PATTERN (insn)) == USE)
+ instantiate_virtual_regs_in_rtx (&PATTERN (insn));
else if (DEBUG_BIND_INSN_P (insn))
instantiate_virtual_regs_in_rtx (INSN_VAR_LOCATION_PTR (insn));
else
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 4a109aee27a..afe1fc836d1 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -1264,6 +1264,25 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p
}
}
+/* Return clobber uses for VARS. */
+
+static gimple_seq
+gimple_build_uses (tree vars)
+{
+ gimple_seq seq = NULL;
+
+ for (tree var = vars; var; var = DECL_CHAIN (var))
+ {
+ if (DECL_ARTIFICIAL (var))
+ continue;
+
+ gimple *stmt = gimple_build_assign (build_clobber (TREE_TYPE (var)), var);
+ gimple_seq_add_stmt (&seq, stmt);
+ }
+
+ return seq;
+}
+
/* Gimplify a BIND_EXPR. Just voidify and recurse. */
static enum gimplify_status
@@ -1363,7 +1382,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
gimplify_seq_add_stmt (&cleanup, stack_restore);
}
- /* Add clobbers for all variables that go out of scope. */
+ /* Add clobbers/uses for all variables that go out of scope. */
for (t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
{
if (VAR_P (t)
@@ -1376,14 +1395,17 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
/* Only care for variables that have to be in memory. Others
will be rewritten into SSA names, hence moved to the
top-level. */
- && !is_gimple_reg (t)
+ && (flag_keep_vars_live || !is_gimple_reg (t))
&& flag_stack_reuse != SR_NONE)
{
tree clobber = build_clobber (TREE_TYPE (t));
- gimple *clobber_stmt;
- clobber_stmt = gimple_build_assign (t, clobber);
- gimple_set_location (clobber_stmt, end_locus);
- gimplify_seq_add_stmt (&cleanup, clobber_stmt);
+ gimple *stmt;
+ if (is_gimple_reg (t))
+ stmt = gimple_build_assign (clobber, t);
+ else
+ stmt = gimple_build_assign (t, clobber);
+ gimple_set_location (stmt, end_locus);
+ gimplify_seq_add_stmt (&cleanup, stmt);
}
if (flag_openacc && oacc_declare_returns != NULL)
@@ -12763,6 +12785,10 @@ gimplify_body (tree fndecl, bool do_parms)
gimple *outer_stmt;
gbind *outer_bind;
+ gimple_seq cleanup = NULL;
+ if (flag_keep_vars_live)
+ cleanup = gimple_build_uses (DECL_ARGUMENTS (fndecl));
+
timevar_push (TV_TREE_GIMPLIFY);
init_tree_ssa (cfun);
@@ -12798,6 +12824,14 @@ gimplify_body (tree fndecl, bool do_parms)
/* Gimplify the function's body. */
seq = NULL;
gimplify_stmt (&DECL_SAVED_TREE (fndecl), &seq);
+
+ if (cleanup)
+ {
+ gtry *gs = gimple_build_try (seq, cleanup, GIMPLE_TRY_FINALLY);
+ seq = NULL;
+ gimplify_seq_add_stmt (&seq, gs);
+ }
+
outer_stmt = gimple_seq_first_stmt (seq);
if (!outer_stmt)
{
diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
index e30e3c92b94..646790af65a 100644
--- a/gcc/testsuite/gcc.dg/guality/pr54200-2.c
+++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
@@ -1,6 +1,5 @@
/* { dg-do run } */
-/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
-/* { dg-options "-g -fdebug-nops -DMAIN" } */
+/* { dg-options "-g -fdebug-nops -fkeep-vars-live -DMAIN" } */
#include "prevent-optimization.h"
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 14d66b7a728..d3a4465fe25 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -4484,6 +4484,7 @@ verify_gimple_assign_single (gassign *stmt)
case PARM_DECL:
if (!is_gimple_reg (lhs)
&& !is_gimple_reg (rhs1)
+ && !TREE_CLOBBER_P (lhs)
&& is_gimple_reg_type (TREE_TYPE (lhs)))
{
error ("invalid rhs for gimple memory store");
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 3e30f6bc3d4..f6488b90378 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -3536,6 +3536,13 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
sra_stats.exprs++;
}
+ if (modify_this_stmt && TREE_CLOBBER_P (gimple_assign_lhs (stmt)))
+ {
+ gimple_assign_set_rhs1 (stmt, rhs);
+ gimple_assign_set_lhs (stmt, build_clobber (TREE_TYPE (rhs)));
+ return SRA_AM_MODIFIED;
+ }
+
if (modify_this_stmt)
{
if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
index 7b25778307f..03f378fa4b2 100644
--- a/gcc/tree-ssa-alias.c
+++ b/gcc/tree-ssa-alias.c
@@ -1368,6 +1368,10 @@ refs_may_alias_p_1 (ao_ref *ref1, ao_ref *ref2, bool tbaa_p)
poly_int64 max_size1 = -1, max_size2 = -1;
bool var1_p, var2_p, ind1_p, ind2_p;
+ if ((ref1->ref && TREE_CLOBBER_P (ref1->ref))
+ || (ref2->ref && TREE_CLOBBER_P (ref2->ref)))
+ return false;
+
gcc_checking_assert ((!ref1->ref
|| TREE_CODE (ref1->ref) == SSA_NAME
|| DECL_P (ref1->ref)
diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index fd24f84fb14..d83579a09c5 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -4883,7 +4883,8 @@ find_func_aliases (struct function *fn, gimple *origt)
tree lhsop = gimple_assign_lhs (t);
tree rhsop = (gimple_num_ops (t) == 2) ? gimple_assign_rhs1 (t) : NULL;
- if (rhsop && TREE_CLOBBER_P (rhsop))
+ if ((rhsop && TREE_CLOBBER_P (rhsop))
+ || (lhsop && TREE_CLOBBER_P (lhsop)))
/* Ignore clobbers, they don't actually store anything into
the LHS. */
;
diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
index 8ccbc85970a..953db9ed02d 100644
--- a/gcc/tree-ssa-uninit.c
+++ b/gcc/tree-ssa-uninit.c
@@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
if (is_gimple_assign (context)
&& gimple_assign_rhs_code (context) == COMPLEX_EXPR)
return;
+ if (gimple_assign_single_p (context)
+ && TREE_CLOBBER_P (gimple_assign_lhs (context)))
+ return;
if (!has_undefined_value_p (t))
return;
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 1/3, debug] Add fdebug-nops
2018-07-24 11:35 ` [RFC 1/3, debug] Add fdebug-nops Tom de Vries
2018-07-24 14:59 ` [PATCH, " Tom de Vries
@ 2018-07-24 19:07 ` Alexandre Oliva
2018-07-24 21:04 ` Tom de Vries
1 sibling, 1 reply; 23+ messages in thread
From: Alexandre Oliva @ 2018-07-24 19:07 UTC (permalink / raw)
To: Tom de Vries; +Cc: Richard Biener, gcc-patches, Jakub Jelinek, Jim Wilson
On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
> There's a design principle in GCC that code generation and debug generation
> are independent. This guarantees that if you're encountering a problem in an
> application without debug info, you can recompile it with -g and be certain
> that you can reproduce the same problem, and use the debug info to debug the
> problem. This invariant is enforced by bootstrap-debug. The fdebug-nops
> breaks this invariant
I thought of a way to not break it: enable the debug info generation
machinery, including VTA and SFN, but discard those only at the very end
if -g is not enabled. The downside is that it would likely slow -Og
down significantly, but who uses it without -g anyway?
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain Engineer Free Software Evangelist
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 2/3, debug] Add fkeep-vars-live
2018-07-24 11:37 ` [RFC 2/3, debug] Add fkeep-vars-live Tom de Vries
2018-07-24 11:46 ` Jakub Jelinek
@ 2018-07-24 19:11 ` Alexandre Oliva
2018-07-25 11:02 ` Jakub Jelinek
1 sibling, 1 reply; 23+ messages in thread
From: Alexandre Oliva @ 2018-07-24 19:11 UTC (permalink / raw)
To: Tom de Vries; +Cc: Richard Biener, gcc-patches, Jakub Jelinek, Jim Wilson
On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
> This patch adds fake uses of user variables at the point where they go out of
> scope, to keep user variables inspectable throughout the application.
I suggest also adding such uses before sets, so that variables aren't
regarded as dead and get optimized out in ranges between the end of a
live range and a subsequent assignment.
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain Engineer Free Software Evangelist
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 1/3, debug] Add fdebug-nops
2018-07-24 19:07 ` [RFC 1/3, " Alexandre Oliva
@ 2018-07-24 21:04 ` Tom de Vries
2018-07-26 7:50 ` Alexandre Oliva
0 siblings, 1 reply; 23+ messages in thread
From: Tom de Vries @ 2018-07-24 21:04 UTC (permalink / raw)
To: Alexandre Oliva; +Cc: Richard Biener, gcc-patches, Jakub Jelinek, Jim Wilson
On 07/24/2018 09:06 PM, Alexandre Oliva wrote:
> On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
>
>> There's a design principle in GCC that code generation and debug generation
>> are independent. This guarantees that if you're encountering a problem in an
>> application without debug info, you can recompile it with -g and be certain
>> that you can reproduce the same problem, and use the debug info to debug the
>> problem. This invariant is enforced by bootstrap-debug. The fdebug-nops
>> breaks this invariant
>
> I thought of a way to not break it: enable the debug info generation
> machinery, including VTA and SFN, but discard those only at the very end
> if -g is not enabled. The downside is that it would likely slow -Og
> down significantly, but who uses it without -g anyway?
I thought of the same. I've submitted a patch here that uses SFN:
https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01391.html . VTA is not
needed AFAIU.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 2/3, debug] Add fkeep-vars-live
2018-07-24 19:11 ` [RFC 2/3, " Alexandre Oliva
@ 2018-07-25 11:02 ` Jakub Jelinek
2018-07-26 7:47 ` Alexandre Oliva
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Jelinek @ 2018-07-25 11:02 UTC (permalink / raw)
To: Alexandre Oliva; +Cc: Tom de Vries, Richard Biener, gcc-patches, Jim Wilson
On Tue, Jul 24, 2018 at 04:11:11PM -0300, Alexandre Oliva wrote:
> On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
>
> > This patch adds fake uses of user variables at the point where they go out of
> > scope, to keep user variables inspectable throughout the application.
>
> I suggest also adding such uses before sets, so that variables aren't
> regarded as dead and get optimized out in ranges between the end of a
> live range and a subsequent assignment.
But that can be done incrementally, right, and perhaps being controllable
by a level of this option, because such extra uses might make it even more
costly. Though, if the extra uses and sets aren't in the same stmt, then
the optimizers could still move it appart (in addition of it making the IL
larger).
We need to think about different cases (non-gimple reg vars are probably ok,
they just live in memory unless converted (sra etc.) into gimple reg vars,
then what to do about SSA_NAMEs with underlying user variable if it doesn't
have overlapping ranges, if it does have overlapping ranges).
Jakub
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH, debug] Add fkeep-vars-live
2018-07-24 15:03 ` [PATCH, " Tom de Vries
@ 2018-07-25 11:06 ` Jakub Jelinek
2018-07-25 11:41 ` Richard Biener
1 sibling, 0 replies; 23+ messages in thread
From: Jakub Jelinek @ 2018-07-25 11:06 UTC (permalink / raw)
To: Tom de Vries; +Cc: Richard Biener, gcc-patches, Jim Wilson, aoliva
On Tue, Jul 24, 2018 at 05:04:06PM +0200, Tom de Vries wrote:
>
> > +Add artificial use for each local variable at the end of the
> > declaration scope
>
> Is this a better option description?
Yes (with a period at the end). Or perhaps "its" instead of "the".
Looks ok to me, just would like to ask one more question, does this prevent
tail-calls or not? If we tail call optimize some call, then I think there
is no need to keep the vars live, because the caller doesn't appear anymore
in the backtrace, on the other side if it is not a tail call, we want to
keep the vars live across the call so that they can be inspected.
Jakub
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH, debug] Add fkeep-vars-live
2018-07-24 15:03 ` [PATCH, " Tom de Vries
2018-07-25 11:06 ` Jakub Jelinek
@ 2018-07-25 11:41 ` Richard Biener
2018-07-25 11:47 ` Richard Biener
1 sibling, 1 reply; 23+ messages in thread
From: Richard Biener @ 2018-07-25 11:41 UTC (permalink / raw)
To: Tom de Vries; +Cc: Jakub Jelinek, gcc-patches, Jim Wilson, aoliva
On Tue, 24 Jul 2018, Tom de Vries wrote:
> On Tue, Jul 24, 2018 at 02:34:26PM +0200, Tom de Vries wrote:
> > On 07/24/2018 01:46 PM, Jakub Jelinek wrote:
> > > On Tue, Jul 24, 2018 at 01:37:32PM +0200, Tom de Vries wrote:
> > >> Another drawback is that the fake uses confuse the unitialized warning
> > >> analysis, so that is switched off for -fkeep-vars-live.
> > >
> > > Is that really needed? I.e. can't you for the purpose of uninitialized
> > > warning analysis ignore the clobber = var uses?
> > >
> >
> > This seems to work on the test-case that failed during testing
> > (g++.dg/uninit-pred-4.C):
> > ...
> > diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> > index 77f090bfa80..953db9ed02d 100644
> > --- a/gcc/tree-ssa-uninit.c
> > +++ b/gcc/tree-ssa-uninit.c
> > @@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr,
> > tree var,
> > if (is_gimple_assign (context)
> > && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
> > return;
> > + if (gimple_assign_single_p (context)
> > + && TREE_CLOBBER_P (gimple_assign_lhs (context)))
> > + return;
> > if (!has_undefined_value_p (t))
> > return;
> > ...
> > But I don't know the pass well enough to know whether this is a
> > sufficient fix.
> >
>
> Updated and re-tested patch.
>
> > +Add artificial use for each local variable at the end of the
> > declaration scope
>
> Is this a better option description?
>
>
> OK for trunk?
>
> Thanks,
> - Tom
>
> [debug] Add fkeep-vars-live
>
> This patch adds fake uses of user variables at the point where they go out of
> scope, to keep user variables inspectable throughout the application.
>
> This approach will generate sub-optimal code: in some cases, the executable
> code will go through efforts to keep a var alive, while var-tracking can
> easily compute the value of the var from something else.
>
> Also, the compiler treats the fake use as any other use, so it may keep an
> expensive resource like a register occupied (if we could mark the use as a
> cold use or some such, we could tell optimizers that we need the value, but
> it's ok if getting the value is expensive, so it could be spilled instead of
> occupying a register).
>
> The current implementation is expected to increase register pressure, and
> therefore spilling, but we'd still expect less memory accesses then with O0.
Few comments inline.
> 2018-07-24 Tom de Vries <tdevries@suse.de>
>
> PR debug/78685
> * cfgexpand.c (expand_use): New function.
> (expand_gimple_stmt_1): Handle TREE_CLOBBER_P as lhs of assignment.
> * common.opt (fkeep-vars-live): New option.
> * function.c (instantiate_virtual_regs): Instantiate in USEs as well.
> * gimplify.c (gimple_build_uses): New function.
> (gimplify_bind_expr): Build clobber uses for variables that don't have
> to be in memory.
> (gimplify_body): Build clobber uses for arguments.
> * tree-cfg.c (verify_gimple_assign_single): Handle TREE_CLOBBER_P as lhs
> of assignment.
> * tree-sra.c (sra_modify_assign): Same.
> * tree-ssa-alias.c (refs_may_alias_p_1): Same.
> * tree-ssa-structalias.c (find_func_aliases): Same.
> * tree-ssa-uninit.c (warn_uninit): Same.
>
> * gcc.dg/guality/pr54200-2.c: Update.
>
> ---
> gcc/cfgexpand.c | 11 ++++++++
> gcc/common.opt | 4 +++
> gcc/function.c | 5 ++--
> gcc/gimplify.c | 46 +++++++++++++++++++++++++++-----
> gcc/testsuite/gcc.dg/guality/pr54200-2.c | 3 +--
> gcc/tree-cfg.c | 1 +
> gcc/tree-sra.c | 7 +++++
> gcc/tree-ssa-alias.c | 4 +++
> gcc/tree-ssa-structalias.c | 3 ++-
> gcc/tree-ssa-uninit.c | 3 +++
> 10 files changed, 76 insertions(+), 11 deletions(-)
>
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index d6e3c382085..e28e8ceec75 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -3533,6 +3533,15 @@ expand_clobber (tree lhs)
> }
> }
>
> +/* Expand a use of RHS. */
> +
> +static void
> +expand_use (tree rhs)
> +{
> + rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> + emit_use (target);
> +}
> +
> /* A subroutine of expand_gimple_stmt, expanding one gimple statement
> STMT that doesn't require special handling for outgoing edges. That
> is no tailcalls and no GIMPLE_COND. */
> @@ -3632,6 +3641,8 @@ expand_gimple_stmt_1 (gimple *stmt)
> /* This is a clobber to mark the going out of scope for
> this LHS. */
> expand_clobber (lhs);
> + else if (TREE_CLOBBER_P (lhs))
> + expand_use (rhs);
> else
> expand_assignment (lhs, rhs,
> gimple_assign_nontemporal_move_p (
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 984b351cd79..a29e320ba45 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -1496,6 +1496,10 @@ fdebug-nops
> Common Report Var(flag_debug_nops) Optimization
> Insert nops if that might improve debugging of optimized code.
>
> +fkeep-vars-live
> +Common Report Var(flag_keep_vars_live) Optimization
> +Add artificial uses of local vars at end of scope.
> +
> fkeep-gc-roots-live
> Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
> ; Always keep a pointer to a live memory block
> diff --git a/gcc/function.c b/gcc/function.c
> index dee303cdbdd..b6aa1eb60d1 100644
> --- a/gcc/function.c
> +++ b/gcc/function.c
> @@ -1964,11 +1964,12 @@ instantiate_virtual_regs (void)
> {
> /* These patterns in the instruction stream can never be recognized.
> Fortunately, they shouldn't contain virtual registers either. */
> - if (GET_CODE (PATTERN (insn)) == USE
> - || GET_CODE (PATTERN (insn)) == CLOBBER
> + if (GET_CODE (PATTERN (insn)) == CLOBBER
> || GET_CODE (PATTERN (insn)) == ASM_INPUT
> || DEBUG_MARKER_INSN_P (insn))
> continue;
> + else if (GET_CODE (PATTERN (insn)) == USE)
> + instantiate_virtual_regs_in_rtx (&PATTERN (insn));
> else if (DEBUG_BIND_INSN_P (insn))
> instantiate_virtual_regs_in_rtx (INSN_VAR_LOCATION_PTR (insn));
> else
> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> index 4a109aee27a..afe1fc836d1 100644
> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -1264,6 +1264,25 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p
> }
> }
>
> +/* Return clobber uses for VARS. */
> +
> +static gimple_seq
> +gimple_build_uses (tree vars)
> +{
> + gimple_seq seq = NULL;
> +
> + for (tree var = vars; var; var = DECL_CHAIN (var))
> + {
> + if (DECL_ARTIFICIAL (var))
I think you want DECL_IGNORED_P here, artificial vars can be refered
to by debug info.
> + continue;
> +
> + gimple *stmt = gimple_build_assign (build_clobber (TREE_TYPE (var)), var);
> + gimple_seq_add_stmt (&seq, stmt);
> + }
> +
> + return seq;
> +}
> +
There's a single call of this function, please inline it.
> /* Gimplify a BIND_EXPR. Just voidify and recurse. */
>
> static enum gimplify_status
> @@ -1363,7 +1382,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> gimplify_seq_add_stmt (&cleanup, stack_restore);
> }
>
> - /* Add clobbers for all variables that go out of scope. */
> + /* Add clobbers/uses for all variables that go out of scope. */
> for (t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
> {
> if (VAR_P (t)
> @@ -1376,14 +1395,17 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> /* Only care for variables that have to be in memory. Others
> will be rewritten into SSA names, hence moved to the
> top-level. */
> - && !is_gimple_reg (t)
> + && (flag_keep_vars_live || !is_gimple_reg (t))
> && flag_stack_reuse != SR_NONE)
> {
> tree clobber = build_clobber (TREE_TYPE (t));
> - gimple *clobber_stmt;
> - clobber_stmt = gimple_build_assign (t, clobber);
> - gimple_set_location (clobber_stmt, end_locus);
> - gimplify_seq_add_stmt (&cleanup, clobber_stmt);
> + gimple *stmt;
> + if (is_gimple_reg (t))
> + stmt = gimple_build_assign (clobber, t);
> + else
> + stmt = gimple_build_assign (t, clobber);
> + gimple_set_location (stmt, end_locus);
> + gimplify_seq_add_stmt (&cleanup, stmt);
> }
>
> if (flag_openacc && oacc_declare_returns != NULL)
> @@ -12763,6 +12785,10 @@ gimplify_body (tree fndecl, bool do_parms)
> gimple *outer_stmt;
> gbind *outer_bind;
>
> + gimple_seq cleanup = NULL;
> + if (flag_keep_vars_live)
> + cleanup = gimple_build_uses (DECL_ARGUMENTS (fndecl));
> +
> timevar_push (TV_TREE_GIMPLIFY);
>
> init_tree_ssa (cfun);
> @@ -12798,6 +12824,14 @@ gimplify_body (tree fndecl, bool do_parms)
> /* Gimplify the function's body. */
> seq = NULL;
> gimplify_stmt (&DECL_SAVED_TREE (fndecl), &seq);
> +
> + if (cleanup)
> + {
> + gtry *gs = gimple_build_try (seq, cleanup, GIMPLE_TRY_FINALLY);
> + seq = NULL;
> + gimplify_seq_add_stmt (&seq, gs);
> + }
> +
> outer_stmt = gimple_seq_first_stmt (seq);
> if (!outer_stmt)
> {
> diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> index e30e3c92b94..646790af65a 100644
> --- a/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> +++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> @@ -1,6 +1,5 @@
> /* { dg-do run } */
> -/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
> -/* { dg-options "-g -fdebug-nops -DMAIN" } */
> +/* { dg-options "-g -fdebug-nops -fkeep-vars-live -DMAIN" } */
>
> #include "prevent-optimization.h"
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index 14d66b7a728..d3a4465fe25 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -4484,6 +4484,7 @@ verify_gimple_assign_single (gassign *stmt)
> case PARM_DECL:
> if (!is_gimple_reg (lhs)
> && !is_gimple_reg (rhs1)
> + && !TREE_CLOBBER_P (lhs)
> && is_gimple_reg_type (TREE_TYPE (lhs)))
> {
> error ("invalid rhs for gimple memory store");
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 3e30f6bc3d4..f6488b90378 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -3536,6 +3536,13 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
> sra_stats.exprs++;
> }
>
> + if (modify_this_stmt && TREE_CLOBBER_P (gimple_assign_lhs (stmt)))
> + {
> + gimple_assign_set_rhs1 (stmt, rhs);
> + gimple_assign_set_lhs (stmt, build_clobber (TREE_TYPE (rhs)));
I think you could modify the type of the lhs in-place since
CONSTRUCTORs are not shared.
> + return SRA_AM_MODIFIED;
> + }
> +
> if (modify_this_stmt)
> {
> if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
> diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> index 7b25778307f..03f378fa4b2 100644
> --- a/gcc/tree-ssa-alias.c
> +++ b/gcc/tree-ssa-alias.c
> @@ -1368,6 +1368,10 @@ refs_may_alias_p_1 (ao_ref *ref1, ao_ref *ref2, bool tbaa_p)
> poly_int64 max_size1 = -1, max_size2 = -1;
> bool var1_p, var2_p, ind1_p, ind2_p;
>
> + if ((ref1->ref && TREE_CLOBBER_P (ref1->ref))
> + || (ref2->ref && TREE_CLOBBER_P (ref2->ref)))
> + return false;
> +
I think it would be better to not arrive here but I assume the uses
are visible as stores with VDEFs in GIMPLE?
> gcc_checking_assert ((!ref1->ref
> || TREE_CODE (ref1->ref) == SSA_NAME
> || DECL_P (ref1->ref)
> diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
> index fd24f84fb14..d83579a09c5 100644
> --- a/gcc/tree-ssa-structalias.c
> +++ b/gcc/tree-ssa-structalias.c
> @@ -4883,7 +4883,8 @@ find_func_aliases (struct function *fn, gimple *origt)
> tree lhsop = gimple_assign_lhs (t);
> tree rhsop = (gimple_num_ops (t) == 2) ? gimple_assign_rhs1 (t) : NULL;
>
> - if (rhsop && TREE_CLOBBER_P (rhsop))
> + if ((rhsop && TREE_CLOBBER_P (rhsop))
> + || (lhsop && TREE_CLOBBER_P (lhsop)))
> /* Ignore clobbers, they don't actually store anything into
> the LHS. */
> ;
> diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> index 8ccbc85970a..953db9ed02d 100644
> --- a/gcc/tree-ssa-uninit.c
> +++ b/gcc/tree-ssa-uninit.c
> @@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
> if (is_gimple_assign (context)
> && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
> return;
> + if (gimple_assign_single_p (context)
> + && TREE_CLOBBER_P (gimple_assign_lhs (context)))
> + return;
> if (!has_undefined_value_p (t))
> return;
I see no handling of uses in DCE so I assume that stmts will be kept
live even though there are no other uses than the artificial USE?
Is that the intention? If so, -Og was explicitely supposed to
elide dead code as much as possible (via CCP + compare&jump folding),
because this not only shrinks code but decreases compile-time. For
the same reason -Og performs inlining.
So I really wonder what class of optimizations we lose with
-fkeep-vars-live. Can you look at testsuite results with
RUNTESTFLAGS="--target_board=unix/\{,-fkeep-vars-live\}"?
Thus compare results with/without -fkeep-vars-live?
Thanks,
Richard.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH, debug] Add fkeep-vars-live
2018-07-25 11:41 ` Richard Biener
@ 2018-07-25 11:47 ` Richard Biener
0 siblings, 0 replies; 23+ messages in thread
From: Richard Biener @ 2018-07-25 11:47 UTC (permalink / raw)
To: Richard Guenther
Cc: tdevries, Jakub Jelinek, GCC Patches, Jim Wilson,
Alexandre Oliva, Vladimir N. Makarov
On Wed, Jul 25, 2018 at 1:41 PM Richard Biener <rguenther@suse.de> wrote:
>
> On Tue, 24 Jul 2018, Tom de Vries wrote:
>
> > On Tue, Jul 24, 2018 at 02:34:26PM +0200, Tom de Vries wrote:
> > > On 07/24/2018 01:46 PM, Jakub Jelinek wrote:
> > > > On Tue, Jul 24, 2018 at 01:37:32PM +0200, Tom de Vries wrote:
> > > >> Another drawback is that the fake uses confuse the unitialized warning
> > > >> analysis, so that is switched off for -fkeep-vars-live.
> > > >
> > > > Is that really needed? I.e. can't you for the purpose of uninitialized
> > > > warning analysis ignore the clobber = var uses?
> > > >
> > >
> > > This seems to work on the test-case that failed during testing
> > > (g++.dg/uninit-pred-4.C):
> > > ...
> > > diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> > > index 77f090bfa80..953db9ed02d 100644
> > > --- a/gcc/tree-ssa-uninit.c
> > > +++ b/gcc/tree-ssa-uninit.c
> > > @@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr,
> > > tree var,
> > > if (is_gimple_assign (context)
> > > && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
> > > return;
> > > + if (gimple_assign_single_p (context)
> > > + && TREE_CLOBBER_P (gimple_assign_lhs (context)))
> > > + return;
> > > if (!has_undefined_value_p (t))
> > > return;
> > > ...
> > > But I don't know the pass well enough to know whether this is a
> > > sufficient fix.
> > >
> >
> > Updated and re-tested patch.
> >
> > > +Add artificial use for each local variable at the end of the
> > > declaration scope
> >
> > Is this a better option description?
> >
> >
> > OK for trunk?
> >
> > Thanks,
> > - Tom
> >
> > [debug] Add fkeep-vars-live
> >
> > This patch adds fake uses of user variables at the point where they go out of
> > scope, to keep user variables inspectable throughout the application.
> >
> > This approach will generate sub-optimal code: in some cases, the executable
> > code will go through efforts to keep a var alive, while var-tracking can
> > easily compute the value of the var from something else.
> >
> > Also, the compiler treats the fake use as any other use, so it may keep an
> > expensive resource like a register occupied (if we could mark the use as a
> > cold use or some such, we could tell optimizers that we need the value, but
> > it's ok if getting the value is expensive, so it could be spilled instead of
> > occupying a register).
> >
> > The current implementation is expected to increase register pressure, and
> > therefore spilling, but we'd still expect less memory accesses then with O0.
>
> Few comments inline.
>
> > 2018-07-24 Tom de Vries <tdevries@suse.de>
> >
> > PR debug/78685
> > * cfgexpand.c (expand_use): New function.
> > (expand_gimple_stmt_1): Handle TREE_CLOBBER_P as lhs of assignment.
> > * common.opt (fkeep-vars-live): New option.
> > * function.c (instantiate_virtual_regs): Instantiate in USEs as well.
> > * gimplify.c (gimple_build_uses): New function.
> > (gimplify_bind_expr): Build clobber uses for variables that don't have
> > to be in memory.
> > (gimplify_body): Build clobber uses for arguments.
> > * tree-cfg.c (verify_gimple_assign_single): Handle TREE_CLOBBER_P as lhs
> > of assignment.
> > * tree-sra.c (sra_modify_assign): Same.
> > * tree-ssa-alias.c (refs_may_alias_p_1): Same.
> > * tree-ssa-structalias.c (find_func_aliases): Same.
> > * tree-ssa-uninit.c (warn_uninit): Same.
> >
> > * gcc.dg/guality/pr54200-2.c: Update.
> >
> > ---
> > gcc/cfgexpand.c | 11 ++++++++
> > gcc/common.opt | 4 +++
> > gcc/function.c | 5 ++--
> > gcc/gimplify.c | 46 +++++++++++++++++++++++++++-----
> > gcc/testsuite/gcc.dg/guality/pr54200-2.c | 3 +--
> > gcc/tree-cfg.c | 1 +
> > gcc/tree-sra.c | 7 +++++
> > gcc/tree-ssa-alias.c | 4 +++
> > gcc/tree-ssa-structalias.c | 3 ++-
> > gcc/tree-ssa-uninit.c | 3 +++
> > 10 files changed, 76 insertions(+), 11 deletions(-)
> >
> > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> > index d6e3c382085..e28e8ceec75 100644
> > --- a/gcc/cfgexpand.c
> > +++ b/gcc/cfgexpand.c
> > @@ -3533,6 +3533,15 @@ expand_clobber (tree lhs)
> > }
> > }
> >
> > +/* Expand a use of RHS. */
> > +
> > +static void
> > +expand_use (tree rhs)
> > +{
> > + rtx target = expand_expr (rhs, NULL_RTX, VOIDmode, EXPAND_NORMAL);
> > + emit_use (target);
> > +}
> > +
> > /* A subroutine of expand_gimple_stmt, expanding one gimple statement
> > STMT that doesn't require special handling for outgoing edges. That
> > is no tailcalls and no GIMPLE_COND. */
> > @@ -3632,6 +3641,8 @@ expand_gimple_stmt_1 (gimple *stmt)
> > /* This is a clobber to mark the going out of scope for
> > this LHS. */
> > expand_clobber (lhs);
> > + else if (TREE_CLOBBER_P (lhs))
> > + expand_use (rhs);
> > else
> > expand_assignment (lhs, rhs,
> > gimple_assign_nontemporal_move_p (
> > diff --git a/gcc/common.opt b/gcc/common.opt
> > index 984b351cd79..a29e320ba45 100644
> > --- a/gcc/common.opt
> > +++ b/gcc/common.opt
> > @@ -1496,6 +1496,10 @@ fdebug-nops
> > Common Report Var(flag_debug_nops) Optimization
> > Insert nops if that might improve debugging of optimized code.
> >
> > +fkeep-vars-live
> > +Common Report Var(flag_keep_vars_live) Optimization
> > +Add artificial uses of local vars at end of scope.
> > +
> > fkeep-gc-roots-live
> > Common Undocumented Report Var(flag_keep_gc_roots_live) Optimization
> > ; Always keep a pointer to a live memory block
> > diff --git a/gcc/function.c b/gcc/function.c
> > index dee303cdbdd..b6aa1eb60d1 100644
> > --- a/gcc/function.c
> > +++ b/gcc/function.c
> > @@ -1964,11 +1964,12 @@ instantiate_virtual_regs (void)
> > {
> > /* These patterns in the instruction stream can never be recognized.
> > Fortunately, they shouldn't contain virtual registers either. */
> > - if (GET_CODE (PATTERN (insn)) == USE
> > - || GET_CODE (PATTERN (insn)) == CLOBBER
> > + if (GET_CODE (PATTERN (insn)) == CLOBBER
> > || GET_CODE (PATTERN (insn)) == ASM_INPUT
> > || DEBUG_MARKER_INSN_P (insn))
> > continue;
> > + else if (GET_CODE (PATTERN (insn)) == USE)
> > + instantiate_virtual_regs_in_rtx (&PATTERN (insn));
> > else if (DEBUG_BIND_INSN_P (insn))
> > instantiate_virtual_regs_in_rtx (INSN_VAR_LOCATION_PTR (insn));
> > else
> > diff --git a/gcc/gimplify.c b/gcc/gimplify.c
> > index 4a109aee27a..afe1fc836d1 100644
> > --- a/gcc/gimplify.c
> > +++ b/gcc/gimplify.c
> > @@ -1264,6 +1264,25 @@ asan_poison_variables (hash_set<tree> *variables, bool poison, gimple_seq *seq_p
> > }
> > }
> >
> > +/* Return clobber uses for VARS. */
> > +
> > +static gimple_seq
> > +gimple_build_uses (tree vars)
> > +{
> > + gimple_seq seq = NULL;
> > +
> > + for (tree var = vars; var; var = DECL_CHAIN (var))
> > + {
> > + if (DECL_ARTIFICIAL (var))
>
> I think you want DECL_IGNORED_P here, artificial vars can be refered
> to by debug info.
>
> > + continue;
> > +
> > + gimple *stmt = gimple_build_assign (build_clobber (TREE_TYPE (var)), var);
> > + gimple_seq_add_stmt (&seq, stmt);
> > + }
> > +
> > + return seq;
> > +}
> > +
>
> There's a single call of this function, please inline it.
>
> > /* Gimplify a BIND_EXPR. Just voidify and recurse. */
> >
> > static enum gimplify_status
> > @@ -1363,7 +1382,7 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> > gimplify_seq_add_stmt (&cleanup, stack_restore);
> > }
> >
> > - /* Add clobbers for all variables that go out of scope. */
> > + /* Add clobbers/uses for all variables that go out of scope. */
> > for (t = BIND_EXPR_VARS (bind_expr); t ; t = DECL_CHAIN (t))
> > {
> > if (VAR_P (t)
> > @@ -1376,14 +1395,17 @@ gimplify_bind_expr (tree *expr_p, gimple_seq *pre_p)
> > /* Only care for variables that have to be in memory. Others
> > will be rewritten into SSA names, hence moved to the
> > top-level. */
> > - && !is_gimple_reg (t)
> > + && (flag_keep_vars_live || !is_gimple_reg (t))
> > && flag_stack_reuse != SR_NONE)
> > {
> > tree clobber = build_clobber (TREE_TYPE (t));
> > - gimple *clobber_stmt;
> > - clobber_stmt = gimple_build_assign (t, clobber);
> > - gimple_set_location (clobber_stmt, end_locus);
> > - gimplify_seq_add_stmt (&cleanup, clobber_stmt);
> > + gimple *stmt;
> > + if (is_gimple_reg (t))
> > + stmt = gimple_build_assign (clobber, t);
> > + else
> > + stmt = gimple_build_assign (t, clobber);
> > + gimple_set_location (stmt, end_locus);
> > + gimplify_seq_add_stmt (&cleanup, stmt);
> > }
> >
> > if (flag_openacc && oacc_declare_returns != NULL)
> > @@ -12763,6 +12785,10 @@ gimplify_body (tree fndecl, bool do_parms)
> > gimple *outer_stmt;
> > gbind *outer_bind;
> >
> > + gimple_seq cleanup = NULL;
> > + if (flag_keep_vars_live)
> > + cleanup = gimple_build_uses (DECL_ARGUMENTS (fndecl));
> > +
> > timevar_push (TV_TREE_GIMPLIFY);
> >
> > init_tree_ssa (cfun);
> > @@ -12798,6 +12824,14 @@ gimplify_body (tree fndecl, bool do_parms)
> > /* Gimplify the function's body. */
> > seq = NULL;
> > gimplify_stmt (&DECL_SAVED_TREE (fndecl), &seq);
> > +
> > + if (cleanup)
> > + {
> > + gtry *gs = gimple_build_try (seq, cleanup, GIMPLE_TRY_FINALLY);
> > + seq = NULL;
> > + gimplify_seq_add_stmt (&seq, gs);
> > + }
> > +
> > outer_stmt = gimple_seq_first_stmt (seq);
> > if (!outer_stmt)
> > {
> > diff --git a/gcc/testsuite/gcc.dg/guality/pr54200-2.c b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> > index e30e3c92b94..646790af65a 100644
> > --- a/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> > +++ b/gcc/testsuite/gcc.dg/guality/pr54200-2.c
> > @@ -1,6 +1,5 @@
> > /* { dg-do run } */
> > -/* { dg-skip-if "" { *-*-* } { "*" } { "-Og" "-Os" "-O0" } } */
> > -/* { dg-options "-g -fdebug-nops -DMAIN" } */
> > +/* { dg-options "-g -fdebug-nops -fkeep-vars-live -DMAIN" } */
> >
> > #include "prevent-optimization.h"
> >
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index 14d66b7a728..d3a4465fe25 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -4484,6 +4484,7 @@ verify_gimple_assign_single (gassign *stmt)
> > case PARM_DECL:
> > if (!is_gimple_reg (lhs)
> > && !is_gimple_reg (rhs1)
> > + && !TREE_CLOBBER_P (lhs)
> > && is_gimple_reg_type (TREE_TYPE (lhs)))
> > {
> > error ("invalid rhs for gimple memory store");
> > diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> > index 3e30f6bc3d4..f6488b90378 100644
> > --- a/gcc/tree-sra.c
> > +++ b/gcc/tree-sra.c
> > @@ -3536,6 +3536,13 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
> > sra_stats.exprs++;
> > }
> >
> > + if (modify_this_stmt && TREE_CLOBBER_P (gimple_assign_lhs (stmt)))
> > + {
> > + gimple_assign_set_rhs1 (stmt, rhs);
> > + gimple_assign_set_lhs (stmt, build_clobber (TREE_TYPE (rhs)));
>
> I think you could modify the type of the lhs in-place since
> CONSTRUCTORs are not shared.
>
> > + return SRA_AM_MODIFIED;
> > + }
> > +
> > if (modify_this_stmt)
> > {
> > if (!useless_type_conversion_p (TREE_TYPE (lhs), TREE_TYPE (rhs)))
> > diff --git a/gcc/tree-ssa-alias.c b/gcc/tree-ssa-alias.c
> > index 7b25778307f..03f378fa4b2 100644
> > --- a/gcc/tree-ssa-alias.c
> > +++ b/gcc/tree-ssa-alias.c
> > @@ -1368,6 +1368,10 @@ refs_may_alias_p_1 (ao_ref *ref1, ao_ref *ref2, bool tbaa_p)
> > poly_int64 max_size1 = -1, max_size2 = -1;
> > bool var1_p, var2_p, ind1_p, ind2_p;
> >
> > + if ((ref1->ref && TREE_CLOBBER_P (ref1->ref))
> > + || (ref2->ref && TREE_CLOBBER_P (ref2->ref)))
> > + return false;
> > +
>
> I think it would be better to not arrive here but I assume the uses
> are visible as stores with VDEFs in GIMPLE?
>
> > gcc_checking_assert ((!ref1->ref
> > || TREE_CODE (ref1->ref) == SSA_NAME
> > || DECL_P (ref1->ref)
> > diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
> > index fd24f84fb14..d83579a09c5 100644
> > --- a/gcc/tree-ssa-structalias.c
> > +++ b/gcc/tree-ssa-structalias.c
> > @@ -4883,7 +4883,8 @@ find_func_aliases (struct function *fn, gimple *origt)
> > tree lhsop = gimple_assign_lhs (t);
> > tree rhsop = (gimple_num_ops (t) == 2) ? gimple_assign_rhs1 (t) : NULL;
> >
> > - if (rhsop && TREE_CLOBBER_P (rhsop))
> > + if ((rhsop && TREE_CLOBBER_P (rhsop))
> > + || (lhsop && TREE_CLOBBER_P (lhsop)))
> > /* Ignore clobbers, they don't actually store anything into
> > the LHS. */
> > ;
> > diff --git a/gcc/tree-ssa-uninit.c b/gcc/tree-ssa-uninit.c
> > index 8ccbc85970a..953db9ed02d 100644
> > --- a/gcc/tree-ssa-uninit.c
> > +++ b/gcc/tree-ssa-uninit.c
> > @@ -132,6 +132,9 @@ warn_uninit (enum opt_code wc, tree t, tree expr, tree var,
> > if (is_gimple_assign (context)
> > && gimple_assign_rhs_code (context) == COMPLEX_EXPR)
> > return;
> > + if (gimple_assign_single_p (context)
> > + && TREE_CLOBBER_P (gimple_assign_lhs (context)))
> > + return;
> > if (!has_undefined_value_p (t))
> > return;
>
> I see no handling of uses in DCE so I assume that stmts will be kept
> live even though there are no other uses than the artificial USE?
> Is that the intention? If so, -Og was explicitely supposed to
> elide dead code as much as possible (via CCP + compare&jump folding),
> because this not only shrinks code but decreases compile-time. For
> the same reason -Og performs inlining.
>
> So I really wonder what class of optimizations we lose with
> -fkeep-vars-live. Can you look at testsuite results with
> RUNTESTFLAGS="--target_board=unix/\{,-fkeep-vars-live\}"?
> Thus compare results with/without -fkeep-vars-live?
Btw, I really wonder if the RA could deal with an extra set of "weak" uses
that enlarge live ranges to a desired point but which doesn't have to be
fulfilled. I suppose "easiest" would be to split the live range at the
real point and add a completely artificial live-range with lower priority
that are only used by a debugger (thus spilling is prefered when no
hardreg is available). I realize infrastructure like DF comes into play here
but I'm more interested in this question from a RA theory perspective
since the RA already should be able to consider "cold" uses vs "hot"
ones?
Richard.
> Thanks,
> Richard.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 2/3, debug] Add fkeep-vars-live
2018-07-25 11:02 ` Jakub Jelinek
@ 2018-07-26 7:47 ` Alexandre Oliva
0 siblings, 0 replies; 23+ messages in thread
From: Alexandre Oliva @ 2018-07-26 7:47 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Tom de Vries, Richard Biener, gcc-patches, Jim Wilson
On Jul 25, 2018, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Jul 24, 2018 at 04:11:11PM -0300, Alexandre Oliva wrote:
>> On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
>>
>> > This patch adds fake uses of user variables at the point where they go out of
>> > scope, to keep user variables inspectable throughout the application.
>>
>> I suggest also adding such uses before sets, so that variables aren't
>> regarded as dead and get optimized out in ranges between the end of a
>> live range and a subsequent assignment.
> But that can be done incrementally, right
Sure
> the optimizers could still move it appart
*nod*
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain Engineer Free Software Evangelist
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [RFC 1/3, debug] Add fdebug-nops
2018-07-24 21:04 ` Tom de Vries
@ 2018-07-26 7:50 ` Alexandre Oliva
0 siblings, 0 replies; 23+ messages in thread
From: Alexandre Oliva @ 2018-07-26 7:50 UTC (permalink / raw)
To: Tom de Vries; +Cc: Richard Biener, gcc-patches, Jakub Jelinek, Jim Wilson
On Jul 24, 2018, Tom de Vries <tdevries@suse.de> wrote:
>> I thought of a way to not break it: enable the debug info generation
>> machinery, including VTA and SFN, but discard those only at the very end
>> if -g is not enabled. The downside is that it would likely slow -Og
>> down significantly, but who uses it without -g anyway?
> I thought of the same. I've submitted a patch here that uses SFN:
> https://gcc.gnu.org/ml/gcc-patches/2018-07/msg01391.html .
Nice!
> VTA is not needed AFAIU.
Yes, indeed. It could avoid inserting some nops, if you were to refrain
from emitting them if there aren't any binds between neighbor SFNs, but
I like it better your way: it's even more like SFN support in the
debugger :-)
--
Alexandre Oliva, freedom fighter https://FSFLA.org/blogs/lxo
Be the change, be Free! FSF Latin America board member
GNU Toolchain Engineer Free Software Evangelist
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-07-26 7:50 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-16 13:29 [RFC][debug] Add -fadd-debug-nops Tom de Vries
2018-07-16 13:34 ` Jakub Jelinek
2018-07-16 14:32 ` Tom de Vries
2018-07-17 1:02 ` Alexandre Oliva
2018-07-16 13:50 ` Richard Biener
2018-07-16 15:10 ` Tom de Vries
2018-07-24 11:30 ` Tom de Vries
2018-07-24 11:35 ` [RFC 1/3, debug] Add fdebug-nops Tom de Vries
2018-07-24 14:59 ` [PATCH, " Tom de Vries
2018-07-24 19:07 ` [RFC 1/3, " Alexandre Oliva
2018-07-24 21:04 ` Tom de Vries
2018-07-26 7:50 ` Alexandre Oliva
2018-07-24 11:37 ` [RFC 2/3, debug] Add fkeep-vars-live Tom de Vries
2018-07-24 11:46 ` Jakub Jelinek
2018-07-24 12:34 ` Tom de Vries
2018-07-24 15:03 ` [PATCH, " Tom de Vries
2018-07-25 11:06 ` Jakub Jelinek
2018-07-25 11:41 ` Richard Biener
2018-07-25 11:47 ` Richard Biener
2018-07-24 19:11 ` [RFC 2/3, " Alexandre Oliva
2018-07-25 11:02 ` Jakub Jelinek
2018-07-26 7:47 ` Alexandre Oliva
2018-07-24 11:38 ` [RFC 3/3, debug] Add fdebug-nops and fkeep-vars-live to Og only Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).