Re: [PATCH] Fix incorrect discriminator assignment.
Andreas Schwab sch...@linux-m68k.org writes:
Rainer Orth r...@cebitec.uni-bielefeld.de writes:
Indeed, and I see -ansi -pedantic-errors -gdwarf-2 -O0 used for the test
on both x86_64-unknown-linux-gnu and i386-pc-solaris2.10 (if I augment
the target clause).
DEFAULT_CFLAGS is set by many *.exp files. dwarf2.exp will not
overwrite an existing definition, so using anthing different from -ansi
-pedantic-errors will not work.
Right, I noticed this myself. Most likely just another one of the many
instances of mindless copy-and-paste programming in our testsuite ;-(
I've now applied the following patch to fix this, after testing on
x86_64-unknown-linux-gnu.
Btw., I've started implementing a dwarf2_debug_line effective-target
keyword for the testsuite.
Rainer
2013-06-07 Rainer Orth r...@cebitec.uni-bielefeld.de
* gcc.dg/debug/dwarf2/discriminator.c: Fix wording.
Revert to dg-options.
diff --git a/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c b/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
--- a/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
+++ b/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
@@ -1,7 +1,7 @@
/* HAVE_AS_DWARF2_DEBUG_LINE macro needs to be defined to pass the unittest.
- However, there dg cannot defin it as, so we restrict the target to linux. */
+ However, dg cannot access it, so we restrict the target to linux. */
/* { dg-do compile { target *-*-linux-gnu } } */
-/* { dg-additional-options -O0 } */
+/* { dg-options -O0 -gdwarf-2 } */
/* { dg-final { scan-assembler loc \[0-9] 11 \[0-9]( is_stmt \[0-9])?\n } } */
/* { dg-final { scan-assembler loc \[0-9] 11 \[0-9]( is_stmt \[0-9])? discriminator 2\n } } */
/* { dg-final { scan-assembler loc \[0-9] 11 \[0-9]( is_stmt \[0-9])? discriminator 1\n } } */
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] Fix incorrect discriminator assignment.
Dehao Chen de...@google.com writes:
On Thu, May 30, 2013 at 10:41 AM, Cary Coutant ccout...@google.com wrote:
That's weird cause in dwarf2.exp:
# If a testcase doesn't have special options, use these.
global DEFAULT_CFLAGS
if ![info exists DEFAULT_CFLAGS] then {
set DEFAULT_CFLAGS -ansi -pedantic-errors -gdwarf-2
}
But anyway, shall I add the -gdwarf-2 option back to discriminator.c?
I think that gets overridden by dg-options. If you use {
dg-additional-options -O2 } instead, it should still pass -gdwarf-2
(along with the other two).
In the current test, I didn't use dg-options, but dg-additional-options
instead:
/* { dg-additional-options -O0 } */
Indeed, and I see -ansi -pedantic-errors -gdwarf-2 -O0 used for the test
on both x86_64-unknown-linux-gnu and i386-pc-solaris2.10 (if I augment
the target clause).
Something unusual seems to be going on with Andreas' testing. Any
RUNTESTFLAGS or DEJAGNU options in the environment?
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] Fix incorrect discriminator assignment.
Andreas Schwab sch...@linux-m68k.org writes: Rainer Orth r...@cebitec.uni-bielefeld.de writes: And why do you add -gdwarf-2 in dg-options? AFAICS all tests in gcc.dg/debug/dwarf2 are built with -gdwarf-2 anyway. Do they? Not here. Executing on host: /daten/aranym/gcc/gcc-20130530/Build/gcc/xgcc -B/daten/aranym/gcc/gcc-20130530/Build/gcc/ /daten/aranym/gcc/gcc-20130530/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c -fno-diagnostics-show-caret -fdiagnostics-color=never -ansi -pedantic-errors -O0 -ffat-lto-objects -ffat-lto-objects -ffat-lto-objects -S -o discriminator.s (timeout = 300) spawn /daten/aranym/gcc/gcc-20130530/Build/gcc/xgcc -B/daten/aranym/gcc/gcc-20130530/Build/gcc/ /daten/aranym/gcc/gcc-20130530/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c -fno-diagnostics-show-caret -fdiagnostics-color=never -ansi -pedantic-errors -O0 -ffat-lto-objects -ffat-lto-objects -ffat-lto-objects -S -o discriminator.s PASS: gcc.dg/debug/dwarf2/discriminator.c (test for excess errors) FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9]( is_stmt [0-9])?\n FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9]( is_stmt [0-9])? discriminator 2\n FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9]( is_stmt [0-9])? discriminator 1\n Could you please run just copy site.exp from one of the testsuite/gcc* directories to a new directory and run % runtest --tool gcc dwarf2.exp=discriminator.c there? Maybe this gives a clue. Thanks. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] Fix incorrect discriminator assignment.
Rainer Orth r...@cebitec.uni-bielefeld.de writes: Indeed, and I see -ansi -pedantic-errors -gdwarf-2 -O0 used for the test on both x86_64-unknown-linux-gnu and i386-pc-solaris2.10 (if I augment the target clause). DEFAULT_CFLAGS is set by many *.exp files. dwarf2.exp will not overwrite an existing definition, so using anthing different from -ansi -pedantic-errors will not work. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: [PATCH] Fix incorrect discriminator assignment.
Rainer Orth r...@cebitec.uni-bielefeld.de writes: And why do you add -gdwarf-2 in dg-options? AFAICS all tests in gcc.dg/debug/dwarf2 are built with -gdwarf-2 anyway. Do they? Not here. Executing on host: /daten/aranym/gcc/gcc-20130530/Build/gcc/xgcc -B/daten/aranym/gcc/gcc-20130530/Build/gcc/ /daten/aranym/gcc/gcc-20130530/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c -fno-diagnostics-show-caret -fdiagnostics-color=never -ansi -pedantic-errors -O0 -ffat-lto-objects -ffat-lto-objects -ffat-lto-objects -S -o discriminator.s(timeout = 300) spawn /daten/aranym/gcc/gcc-20130530/Build/gcc/xgcc -B/daten/aranym/gcc/gcc-20130530/Build/gcc/ /daten/aranym/gcc/gcc-20130530/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c -fno-diagnostics-show-caret -fdiagnostics-color=never -ansi -pedantic-errors -O0 -ffat-lto-objects -ffat-lto-objects -ffat-lto-objects -S -o discriminator.s PASS: gcc.dg/debug/dwarf2/discriminator.c (test for excess errors) FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9]( is_stmt [0-9])?\n FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9]( is_stmt [0-9])? discriminator 2\n FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9]( is_stmt [0-9])? discriminator 1\n Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different.
Re: [PATCH] Fix incorrect discriminator assignment.
On Thu, May 30, 2013 at 1:40 AM, Andreas Schwab sch...@linux-m68k.org wrote:
Rainer Orth r...@cebitec.uni-bielefeld.de writes:
And why do you add -gdwarf-2 in dg-options? AFAICS all tests in
gcc.dg/debug/dwarf2 are built with -gdwarf-2 anyway.
Do they? Not here.
Executing on host: /daten/aranym/gcc/gcc-20130530/Build/gcc/xgcc
-B/daten/aranym/gcc/gcc-20130530/Build/gcc/
/daten/aranym/gcc/gcc-20130530/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
-fno-diagnostics-show-caret -fdiagnostics-color=never -ansi
-pedantic-errors -O0 -ffat-lto-objects -ffat-lto-objects -ffat-lto-objects -S
-o discriminator.s(timeout = 300)
spawn /daten/aranym/gcc/gcc-20130530/Build/gcc/xgcc
-B/daten/aranym/gcc/gcc-20130530/Build/gcc/
/daten/aranym/gcc/gcc-20130530/gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
-fno-diagnostics-show-caret -fdiagnostics-color=never -ansi -pedantic-errors
-O0 -ffat-lto-objects -ffat-lto-objects -ffat-lto-objects -S -o
discriminator.s
PASS: gcc.dg/debug/dwarf2/discriminator.c (test for excess errors)
FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9](
is_stmt [0-9])?\n
FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9](
is_stmt [0-9])? discriminator 2\n
FAIL: gcc.dg/debug/dwarf2/discriminator.c scan-assembler loc [0-9] 11 [0-9](
is_stmt [0-9])? discriminator 1\n
That's weird cause in dwarf2.exp:
# If a testcase doesn't have special options, use these.
global DEFAULT_CFLAGS
if ![info exists DEFAULT_CFLAGS] then {
set DEFAULT_CFLAGS -ansi -pedantic-errors -gdwarf-2
}
But anyway, shall I add the -gdwarf-2 option back to discriminator.c?
Dehao
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
Re: [PATCH] Fix incorrect discriminator assignment.
That's weird cause in dwarf2.exp:
# If a testcase doesn't have special options, use these.
global DEFAULT_CFLAGS
if ![info exists DEFAULT_CFLAGS] then {
set DEFAULT_CFLAGS -ansi -pedantic-errors -gdwarf-2
}
But anyway, shall I add the -gdwarf-2 option back to discriminator.c?
I think that gets overridden by dg-options. If you use {
dg-additional-options -O2 } instead, it should still pass -gdwarf-2
(along with the other two).
-cary
Re: [PATCH] Fix incorrect discriminator assignment.
On Thu, May 30, 2013 at 10:41 AM, Cary Coutant ccout...@google.com wrote:
That's weird cause in dwarf2.exp:
# If a testcase doesn't have special options, use these.
global DEFAULT_CFLAGS
if ![info exists DEFAULT_CFLAGS] then {
set DEFAULT_CFLAGS -ansi -pedantic-errors -gdwarf-2
}
But anyway, shall I add the -gdwarf-2 option back to discriminator.c?
I think that gets overridden by dg-options. If you use {
dg-additional-options -O2 } instead, it should still pass -gdwarf-2
(along with the other two).
In the current test, I didn't use dg-options, but dg-additional-options instead:
/* { dg-additional-options -O0 } */
Dehao
-cary
Re: [PATCH] Fix incorrect discriminator assignment.
Mike Stump m...@mrs.kithrup.com writes:
On May 28, 2013, at 3:56 PM, Dehao Chen de...@google.com wrote:
The attached patch restricts the test only run on linux-gnu targets. Is it
ok?
Ok. You can put HAVE_AS_DWARF2_DEBUG_LINE in a comment above it to help
explain why the target restriction applies. The idea is, if enough people
care, or some x86 target (say, darwin), want to reliably find them all, one
can then search for that and fix them all in one go.
Right, this does work e.g. on Solaris/x86 with gas, but fails with Sun
as or Darwin as.
Btw., why is this even x86 specific? At least on Solaris/SPARC with
gas, HAVE_AS_DWARF2_DEBUG_LINE *is* defined.
And why do you add -gdwarf-2 in dg-options? AFAICS all tests in
gcc.dg/debug/dwarf2 are built with -gdwarf-2 anyway. Perhaps you need
just { dg-additional-options -O0 } instead?
Also, please mention PR testsuite/57413 in the ChangeLog and move the
addition of the testcase from gcc/ChangeLog to gcc/testsuite/ChangeLog,
removing the testsuite/ prefix in the pathname.
Thanks.
Rainer
--
-
Rainer Orth, Center for Biotechnology, Bielefeld University
Re: [PATCH] Fix incorrect discriminator assignment.
Patch updated and committed as r199412. Thanks, Dehao
Re: [PATCH] Fix incorrect discriminator assignment.
ChangeLog fix patch committed in r199394.
The attached patch restricts the test only run on linux-gnu targets. Is it ok?
Thanks
Dehao
gcc/ChangeLog:
2013-05-28 Dehao Chen de...@google.com
testsuite/debug/dwarf2/discriminator.c: Restrict the test to linux only.
Index: gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
===
--- gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (revision 199393)
+++ gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (working copy)
@@ -1,4 +1,4 @@
-/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-do compile { target { i?86-*-linux-gnu x86_64-*-linux-gnu } } } */
/* { dg-options -O0 -gdwarf-2 } */
/* { dg-final { scan-assembler loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?\n } } */
/* { dg-final { scan-assembler loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?
discriminator 2\n } } */
Re: [PATCH] Fix incorrect discriminator assignment.
The attached patch restricts the test only run on linux-gnu targets. Is it ok?
Ideally we would restrict the test to those targets where
HAVE_AS_DWARF2_DEBUG_LINE is set, but I don't think that can be done
in dg. OK if no one suggests a way to do exactly that.
-cary
gcc/ChangeLog:
2013-05-28 Dehao Chen de...@google.com
testsuite/debug/dwarf2/discriminator.c: Restrict the test to linux only.
Index: gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
===
--- gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (revision 199393)
+++ gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (working copy)
@@ -1,4 +1,4 @@
-/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-do compile { target { i?86-*-linux-gnu x86_64-*-linux-gnu } } } */
/* { dg-options -O0 -gdwarf-2 } */
/* { dg-final { scan-assembler loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?\n } }
*/
/* { dg-final { scan-assembler loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?
discriminator 2\n } } */
Re: [PATCH] Fix incorrect discriminator assignment.
On May 28, 2013, at 3:56 PM, Dehao Chen de...@google.com wrote: The attached patch restricts the test only run on linux-gnu targets. Is it ok? Ok. You can put HAVE_AS_DWARF2_DEBUG_LINE in a comment above it to help explain why the target restriction applies. The idea is, if enough people care, or some x86 target (say, darwin), want to reliably find them all, one can then search for that and fix them all in one go.
Re: [PATCH] Fix incorrect discriminator assignment.
Sure, will update the patch for that. ... This cause pr57413. Dominique PS the escaping in the regexp seems strange: \[0-9]
Re: [PATCH] Fix incorrect discriminator assignment.
gcc/ChangeLog: * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. (locus_descrim_hasher::equal): Likewise. (next_discriminator_for_locus): Likewise. (assign_discriminator): Add return value. (make_edges): Assign more discriminator if necessary. (make_cond_expr_edges): Likewise. (make_goto_expr_edges): Likewise. gcc/testsuite/ChangeLog: * gcc.dg/debug/dwarf2/discriminator.c: New Test. The above ChangeLog entries are good, but the ones you installed aren't (wrong gcc/ prefix, wrong location for the testsuite entry, several typos, etc). Please fix. -- Eric Botcazou
Re: [PATCH] Fix incorrect discriminator assignment.
Sounds reasonable. The patch is updated, bootstrapped and passed all
regression test.
Dehao
On Tue, May 21, 2013 at 5:34 AM, Richard Biener
richard.guent...@gmail.com wrote:
On Fri, May 17, 2013 at 5:48 PM, Dehao Chen de...@google.com wrote:
On Fri, May 17, 2013 at 1:22 AM, Richard Biener
richard.guent...@gmail.com wrote:
On Wed, May 15, 2013 at 6:50 PM, Cary Coutant ccout...@google.com wrote:
gcc/ChangeLog:
* tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
(locus_descrim_hasher::equal): Likewise.
(next_discriminator_for_locus): Likewise.
(assign_discriminator): Add return value.
(make_edges): Assign more discriminator if necessary.
(make_cond_expr_edges): Likewise.
(make_goto_expr_edges): Likewise.
gcc/testsuite/ChangeLog:
* gcc.dg/debug/dwarf2/discriminator.c: New Test.
Looks OK to me, but I can't approve patches for tree-cfg.c.
Two comments:
(1) In the C++ conversion, it looks like someone misspelled discrim
in locus_descrim_hasher.
(2) Is this worth fixing in trunk when we'll eventually switch to a
per-instruction discriminator?
This patch fixes a common case where one line is distributed to 3 BBs,
but only 1 discriminator is assigned.
As far as I can see the patch makes discriminators coarser (by hashing
and comparing on LOCATION_LINE and not on the location). It also has
the changes like
- assign_discriminator (entry_locus, then_bb);
+ if (assign_discriminator (entry_locus, then_bb))
+assign_discriminator (entry_locus, bb);
which is what the comment refers to? I think those changes are
short-sighted
because what happens if the 2nd assign_discriminator call has exactly the
same issue? Especially with the make_goto_expr_edges change there
may be multiple predecessors where I cannot see how the change is
correct. Yes, the change changes something and thus may fix the testcase
but it doesn't change things in a predictable way as far as I can see.
So - the change to compare/hash LOCATION_LINE looks good to me,
but the assign_discriminator change needs more explaining.
The new discriminator assignment algorithm is:
1 FOR_EACH_BB:
2 FOR_EACH_SUCC_EDGE:
3 if (same_line(bb, succ_bb))
4 if (succ_bb has no discriminator)
5 succ_bb-discriminator = new_discriminator
6 else if (bb has no discriminator)
7 bb-discriminator = new_discriminator
Because there are so many places to handle SUCC_EDGE, thus the logic
line #3, #4 is embedded within assign_discriminator function, and the
logic in line #6 is encoded as the return value of
assign_discriminator.
Hmm, as of current the code is hardly readable while the above makes sense
(well, apart from what happens if both succ and bb already have a
discriminator).
Can I make you refactor the current code to postpone discriminator assignment
until after make_edges completed, thus, do it in a postprocessing step done
exactly like outlined above? That way also the whole thing, including
the currently global discriminator_per_locus can be localized into a single
function.
Thanks,
Richard.
Thanks,
Dehao
Richard.
-cary
Index: gcc/tree-cfg.c
===
--- gcc/tree-cfg.c (revision 198891)
+++ gcc/tree-cfg.c (working copy)
@@ -105,7 +105,7 @@ struct locus_descrim_hasher : typed_free_remove l
inline hashval_t
locus_descrim_hasher::hash (const value_type *item)
{
- return item-locus;
+ return LOCATION_LINE (item-locus);
}
/* Equality function for the locus-to-discriminator map. A and B
@@ -114,7 +114,7 @@ locus_descrim_hasher::hash (const value_type *item
inline bool
locus_descrim_hasher::equal (const value_type *a, const compare_type *b)
{
- return a-locus == b-locus;
+ return LOCATION_LINE (a-locus) == LOCATION_LINE (b-locus);
}
static hash_table locus_descrim_hasher discriminator_per_locus;
@@ -125,11 +125,11 @@ static void factor_computed_gotos (void);
/* Edges. */
static void make_edges (void);
+static void assign_discriminators (void);
static void make_cond_expr_edges (basic_block);
static void make_gimple_switch_edges (basic_block);
static void make_goto_expr_edges (basic_block);
static void make_gimple_asm_edges (basic_block);
-static void assign_discriminator (location_t, basic_block);
static edge gimple_redirect_edge_and_branch (edge, basic_block);
static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
static unsigned int split_critical_edges (void);
@@ -231,6 +231,7 @@ build_gimple_cfg (gimple_seq seq)
/* Create the edges of the flowgraph. */
discriminator_per_locus.create (13);
make_edges ();
+ assign_discriminators ();
cleanup_dead_labels ();
discriminator_per_locus.dispose ();
}
@@ -690,11 +691,7 @@ make_edges (void)
fallthru = true;
if (fallthru)
-{
- make_edge (bb, bb-next_bb, EDGE_FALLTHRU);
- if (last)
-assign_discriminator (gimple_location
Re: [PATCH] Fix incorrect discriminator assignment.
@@ -105,7 +105,7 @@ struct locus_descrim_hasher : typed_free_remove l
inline hashval_t
locus_descrim_hasher::hash (const value_type *item)
{
- return item-locus;
+ return LOCATION_LINE (item-locus);
}
/* Equality function for the locus-to-discriminator map. A and B
@@ -114,7 +114,7 @@ locus_descrim_hasher::hash (const value_type *item
inline bool
locus_descrim_hasher::equal (const value_type *a, const compare_type *b)
{
- return a-locus == b-locus;
+ return LOCATION_LINE (a-locus) == LOCATION_LINE (b-locus);
}
While you're in that part of the code, could you fix the spelling of
locus_descrim_hasher? Should be locus_discrim_hasher.
-cary
Re: [PATCH] Fix incorrect discriminator assignment.
Sure, will update the patch for that.
Dehao
On Wed, May 22, 2013 at 9:40 AM, Cary Coutant ccout...@google.com wrote:
@@ -105,7 +105,7 @@ struct locus_descrim_hasher : typed_free_remove l
inline hashval_t
locus_descrim_hasher::hash (const value_type *item)
{
- return item-locus;
+ return LOCATION_LINE (item-locus);
}
/* Equality function for the locus-to-discriminator map. A and B
@@ -114,7 +114,7 @@ locus_descrim_hasher::hash (const value_type *item
inline bool
locus_descrim_hasher::equal (const value_type *a, const compare_type *b)
{
- return a-locus == b-locus;
+ return LOCATION_LINE (a-locus) == LOCATION_LINE (b-locus);
}
While you're in that part of the code, could you fix the spelling of
locus_descrim_hasher? Should be locus_discrim_hasher.
-cary
Re: [PATCH] Fix incorrect discriminator assignment.
On Fri, May 17, 2013 at 5:48 PM, Dehao Chen de...@google.com wrote: On Fri, May 17, 2013 at 1:22 AM, Richard Biener richard.guent...@gmail.com wrote: On Wed, May 15, 2013 at 6:50 PM, Cary Coutant ccout...@google.com wrote: gcc/ChangeLog: * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. (locus_descrim_hasher::equal): Likewise. (next_discriminator_for_locus): Likewise. (assign_discriminator): Add return value. (make_edges): Assign more discriminator if necessary. (make_cond_expr_edges): Likewise. (make_goto_expr_edges): Likewise. gcc/testsuite/ChangeLog: * gcc.dg/debug/dwarf2/discriminator.c: New Test. Looks OK to me, but I can't approve patches for tree-cfg.c. Two comments: (1) In the C++ conversion, it looks like someone misspelled discrim in locus_descrim_hasher. (2) Is this worth fixing in trunk when we'll eventually switch to a per-instruction discriminator? This patch fixes a common case where one line is distributed to 3 BBs, but only 1 discriminator is assigned. As far as I can see the patch makes discriminators coarser (by hashing and comparing on LOCATION_LINE and not on the location). It also has the changes like - assign_discriminator (entry_locus, then_bb); + if (assign_discriminator (entry_locus, then_bb)) +assign_discriminator (entry_locus, bb); which is what the comment refers to? I think those changes are short-sighted because what happens if the 2nd assign_discriminator call has exactly the same issue? Especially with the make_goto_expr_edges change there may be multiple predecessors where I cannot see how the change is correct. Yes, the change changes something and thus may fix the testcase but it doesn't change things in a predictable way as far as I can see. So - the change to compare/hash LOCATION_LINE looks good to me, but the assign_discriminator change needs more explaining. The new discriminator assignment algorithm is: 1 FOR_EACH_BB: 2 FOR_EACH_SUCC_EDGE: 3 if (same_line(bb, succ_bb)) 4 if (succ_bb has no discriminator) 5 succ_bb-discriminator = new_discriminator 6 else if (bb has no discriminator) 7 bb-discriminator = new_discriminator Because there are so many places to handle SUCC_EDGE, thus the logic line #3, #4 is embedded within assign_discriminator function, and the logic in line #6 is encoded as the return value of assign_discriminator. Hmm, as of current the code is hardly readable while the above makes sense (well, apart from what happens if both succ and bb already have a discriminator). Can I make you refactor the current code to postpone discriminator assignment until after make_edges completed, thus, do it in a postprocessing step done exactly like outlined above? That way also the whole thing, including the currently global discriminator_per_locus can be localized into a single function. Thanks, Richard. Thanks, Dehao Richard. -cary
Re: [PATCH] Fix incorrect discriminator assignment.
On Wed, May 15, 2013 at 6:50 PM, Cary Coutant ccout...@google.com wrote: gcc/ChangeLog: * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. (locus_descrim_hasher::equal): Likewise. (next_discriminator_for_locus): Likewise. (assign_discriminator): Add return value. (make_edges): Assign more discriminator if necessary. (make_cond_expr_edges): Likewise. (make_goto_expr_edges): Likewise. gcc/testsuite/ChangeLog: * gcc.dg/debug/dwarf2/discriminator.c: New Test. Looks OK to me, but I can't approve patches for tree-cfg.c. Two comments: (1) In the C++ conversion, it looks like someone misspelled discrim in locus_descrim_hasher. (2) Is this worth fixing in trunk when we'll eventually switch to a per-instruction discriminator? This patch fixes a common case where one line is distributed to 3 BBs, but only 1 discriminator is assigned. As far as I can see the patch makes discriminators coarser (by hashing and comparing on LOCATION_LINE and not on the location). It also has the changes like - assign_discriminator (entry_locus, then_bb); + if (assign_discriminator (entry_locus, then_bb)) +assign_discriminator (entry_locus, bb); which is what the comment refers to? I think those changes are short-sighted because what happens if the 2nd assign_discriminator call has exactly the same issue? Especially with the make_goto_expr_edges change there may be multiple predecessors where I cannot see how the change is correct. Yes, the change changes something and thus may fix the testcase but it doesn't change things in a predictable way as far as I can see. So - the change to compare/hash LOCATION_LINE looks good to me, but the assign_discriminator change needs more explaining. Richard. -cary
Re: [PATCH] Fix incorrect discriminator assignment.
On Fri, May 17, 2013 at 1:22 AM, Richard Biener richard.guent...@gmail.com wrote: On Wed, May 15, 2013 at 6:50 PM, Cary Coutant ccout...@google.com wrote: gcc/ChangeLog: * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. (locus_descrim_hasher::equal): Likewise. (next_discriminator_for_locus): Likewise. (assign_discriminator): Add return value. (make_edges): Assign more discriminator if necessary. (make_cond_expr_edges): Likewise. (make_goto_expr_edges): Likewise. gcc/testsuite/ChangeLog: * gcc.dg/debug/dwarf2/discriminator.c: New Test. Looks OK to me, but I can't approve patches for tree-cfg.c. Two comments: (1) In the C++ conversion, it looks like someone misspelled discrim in locus_descrim_hasher. (2) Is this worth fixing in trunk when we'll eventually switch to a per-instruction discriminator? This patch fixes a common case where one line is distributed to 3 BBs, but only 1 discriminator is assigned. As far as I can see the patch makes discriminators coarser (by hashing and comparing on LOCATION_LINE and not on the location). It also has the changes like - assign_discriminator (entry_locus, then_bb); + if (assign_discriminator (entry_locus, then_bb)) +assign_discriminator (entry_locus, bb); which is what the comment refers to? I think those changes are short-sighted because what happens if the 2nd assign_discriminator call has exactly the same issue? Especially with the make_goto_expr_edges change there may be multiple predecessors where I cannot see how the change is correct. Yes, the change changes something and thus may fix the testcase but it doesn't change things in a predictable way as far as I can see. So - the change to compare/hash LOCATION_LINE looks good to me, but the assign_discriminator change needs more explaining. The new discriminator assignment algorithm is: 1 FOR_EACH_BB: 2 FOR_EACH_SUCC_EDGE: 3 if (same_line(bb, succ_bb)) 4 if (succ_bb has no discriminator) 5 succ_bb-discriminator = new_discriminator 6 else if (bb has no discriminator) 7 bb-discriminator = new_discriminator Because there are so many places to handle SUCC_EDGE, thus the logic line #3, #4 is embedded within assign_discriminator function, and the logic in line #6 is encoded as the return value of assign_discriminator. Thanks, Dehao Richard. -cary
[PATCH] Fix incorrect discriminator assignment.
This patch fixes a common case where one line is distributed to 3 BBs,
but only 1 discriminator is assigned.
Bootstrapped and passed gcc regression test.
OK for trunk?
Thanks,
Dehao
gcc/ChangeLog:
* tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno.
(locus_descrim_hasher::equal): Likewise.
(next_discriminator_for_locus): Likewise.
(assign_discriminator): Add return value.
(make_edges): Assign more discriminator if necessary.
(make_cond_expr_edges): Likewise.
(make_goto_expr_edges): Likewise.
gcc/testsuite/ChangeLog:
* gcc.dg/debug/dwarf2/discriminator.c: New Test.
Index: gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c
===
--- gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (revision 0)
+++ gcc/testsuite/gcc.dg/debug/dwarf2/discriminator.c (revision 0)
@@ -0,0 +1,16 @@
+/* { dg-do compile { target { i?86-*-* x86_64-*-* } } } */
+/* { dg-options -O0 -gdwarf-2 } */
+/* { dg-final { scan-assembler loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?\n } } */
+/* { dg-final { scan-assembler loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?
discriminator 2\n } } */
+/* { dg-final { scan-assembler loc \[0-9] 9 \[0-9]( is_stmt \[0-9])?
discriminator 1\n } } */
+
+int foo(int n) {
+ int i, ret = 0;
+ for (i = 0; i n; i++) {
+if (i % 10 == 1)
+ ret++;
+else
+ ret--;
+ }
+ return ret;
+}
Index: gcc/tree-cfg.c
===
--- gcc/tree-cfg.c (revision 198891)
+++ gcc/tree-cfg.c (working copy)
@@ -105,7 +105,7 @@ struct locus_descrim_hasher : typed_free_remove l
inline hashval_t
locus_descrim_hasher::hash (const value_type *item)
{
- return item-locus;
+ return LOCATION_LINE (item-locus);
}
/* Equality function for the locus-to-discriminator map. A and B
@@ -114,7 +114,7 @@ locus_descrim_hasher::hash (const value_type *item
inline bool
locus_descrim_hasher::equal (const value_type *a, const compare_type *b)
{
- return a-locus == b-locus;
+ return LOCATION_LINE (a-locus) == LOCATION_LINE (b-locus);
}
static hash_table locus_descrim_hasher discriminator_per_locus;
@@ -129,7 +129,7 @@ static void make_cond_expr_edges (basic_block);
static void make_gimple_switch_edges (basic_block);
static void make_goto_expr_edges (basic_block);
static void make_gimple_asm_edges (basic_block);
-static void assign_discriminator (location_t, basic_block);
+static bool assign_discriminator (location_t, basic_block);
static edge gimple_redirect_edge_and_branch (edge, basic_block);
static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
static unsigned int split_critical_edges (void);
@@ -693,7 +693,8 @@ make_edges (void)
{
make_edge (bb, bb-next_bb, EDGE_FALLTHRU);
if (last)
-assign_discriminator (gimple_location (last), bb-next_bb);
+if (assign_discriminator (gimple_location (last), bb-next_bb))
+ assign_discriminator (gimple_location (last), bb);
}
}
@@ -717,7 +718,8 @@ next_discriminator_for_locus (location_t locus)
item.locus = locus;
item.discriminator = 0;
- slot = discriminator_per_locus.find_slot_with_hash (item, locus, INSERT);
+ slot = discriminator_per_locus.find_slot_with_hash (
+ item, LOCATION_LINE (locus), INSERT);
gcc_assert (slot);
if (*slot == HTAB_EMPTY_ENTRY)
{
@@ -753,21 +755,29 @@ same_line_p (location_t locus1, location_t locus2)
}
/* Assign a unique discriminator value to block BB if it begins at the same
- LOCUS as its predecessor block. */
+ LOCUS as its pred/succ block. Return true if discriminator has already
+ been assigned for BB and needs to assign additional discriminator for
+ BB's predecessor. */
-static void
+static bool
assign_discriminator (location_t locus, basic_block bb)
{
gimple first_in_to_bb, last_in_to_bb;
- if (locus == 0 || bb-discriminator != 0)
-return;
+ if (locus == 0)
+return false;
first_in_to_bb = first_non_label_stmt (bb);
last_in_to_bb = last_stmt (bb);
if ((first_in_to_bb same_line_p (locus, gimple_location (first_in_to_bb)))
|| (last_in_to_bb same_line_p (locus, gimple_location
(last_in_to_bb
-bb-discriminator = next_discriminator_for_locus (locus);
+{
+ if (bb-discriminator != 0)
+ return true;
+ else
+ bb-discriminator = next_discriminator_for_locus (locus);
+}
+ return false;
}
/* Create the edges for a GIMPLE_COND starting at block BB. */
@@ -796,12 +806,14 @@ make_cond_expr_edges (basic_block bb)
else_stmt = first_stmt (else_bb);
e = make_edge (bb, then_bb, EDGE_TRUE_VALUE);
- assign_discriminator (entry_locus, then_bb);
+ if (assign_discriminator (entry_locus, then_bb))
+assign_discriminator (entry_locus, bb);
e-goto_locus = gimple_location (then_stmt);
e = make_edge (bb, else_bb, EDGE_FALSE_VALUE);
if (e)
{
- assign_discriminator (entry_locus, else_bb);
+
Re: [PATCH] Fix incorrect discriminator assignment.
gcc/ChangeLog: * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. (locus_descrim_hasher::equal): Likewise. (next_discriminator_for_locus): Likewise. (assign_discriminator): Add return value. (make_edges): Assign more discriminator if necessary. (make_cond_expr_edges): Likewise. (make_goto_expr_edges): Likewise. gcc/testsuite/ChangeLog: * gcc.dg/debug/dwarf2/discriminator.c: New Test. Looks OK to me, but I can't approve patches for tree-cfg.c. Two comments: (1) In the C++ conversion, it looks like someone misspelled discrim in locus_descrim_hasher. (2) Is this worth fixing in trunk when we'll eventually switch to a per-instruction discriminator? -cary
Re: [PATCH] Fix incorrect discriminator assignment.
On Wed, May 15, 2013 at 9:50 AM, Cary Coutant ccout...@google.com wrote: gcc/ChangeLog: * tree-cfg.c (locus_descrim_hasher::hash): Only hash lineno. (locus_descrim_hasher::equal): Likewise. (next_discriminator_for_locus): Likewise. (assign_discriminator): Add return value. (make_edges): Assign more discriminator if necessary. (make_cond_expr_edges): Likewise. (make_goto_expr_edges): Likewise. gcc/testsuite/ChangeLog: * gcc.dg/debug/dwarf2/discriminator.c: New Test. Looks OK to me, but I can't approve patches for tree-cfg.c. Two comments: (1) In the C++ conversion, it looks like someone misspelled discrim in locus_descrim_hasher. (2) Is this worth fixing in trunk when we'll eventually switch to a per-instruction discriminator? Yeah, the same problem exists for per-instruction discriminator. This patch needs to be backported to google-4_7 and 4_8 branches. Thanks, Dehao -cary
