[PATCH] skip debug stmts when assigning locus discriminators

[PATCH] skip debug stmts when assigning locus discriminators

[Alexandre Oliva]

c-c++-common/goacc/kernels-loop-g.c has been failing (compare-debug)
on i686-linux-gnu since r13-3172, because the implementation enabled
debug stmts to cause discriminators to be assigned differently, and
the discriminators are printed in the .gkd dumps that -fcompare-debug
compares.

This patch prevents debug stmts from affecting the discriminators in
nondebug stmts, but enables debug stmts to get discriminators just as
nondebug stmts would if their line numbers match.

I suppose we could arrange for discriminators to be omitted from the
-fcompare-debug dumps, but keeping discriminators in sync is probably
good to avoid other potential sources of divergence between debug and
nondebug.

Regstrapped on x86_64-linux-gnu, also tested with gcc-13 on i686- and
x86_64-.  Ok to install?

(Eugene, I suppose what's special about this testcase, that may not
apply to most other uses of assign_discriminators, is that goacc creates
new functions out of already optimized code.  I think
assign_discriminators may not be suitable for new functions, with code
that isn't exactly pristinely in-order.  WDYT?)


for  gcc/ChangeLog

	* tree-cfg.cc (assign_discriminators): Handle debug stmts.
---
 gcc/tree-cfg.cc |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index 40a6f2a3b529f..a30a2de33a106 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -1214,6 +1214,22 @@ assign_discriminators (void)
 	{
 	  gimple *stmt = gsi_stmt (gsi);
 
+	  /* Don't allow debug stmts to affect discriminators, but
+	     allow them to take discriminators when they're on the
+	     same line as the preceding nondebug stmt.  */
+	  if (is_gimple_debug (stmt))
+	    {
+	      if (curr_locus != UNKNOWN_LOCATION
+		  && same_line_p (curr_locus, &curr_locus_e,
+				  gimple_location (stmt)))
+		{
+		  location_t loc = gimple_location (stmt);
+		  location_t dloc = location_with_discriminator (loc,
+								 curr_discr);
+		  gimple_set_location (stmt, dloc);
+		}
+	      continue;
+	    }
 	  if (curr_locus == UNKNOWN_LOCATION)
 	    {
 	      curr_locus = gimple_location (stmt);

-- 
Alexandre Oliva, happy hacker            https://FSFLA.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive

Re: [PATCH] skip debug stmts when assigning locus discriminators

[Jeff Law]

On 11/8/23 08:51, Alexandre Oliva wrote:
> 
> c-c++-common/goacc/kernels-loop-g.c has been failing (compare-debug)
> on i686-linux-gnu since r13-3172, because the implementation enabled
> debug stmts to cause discriminators to be assigned differently, and
> the discriminators are printed in the .gkd dumps that -fcompare-debug
> compares.
> 
> This patch prevents debug stmts from affecting the discriminators in
> nondebug stmts, but enables debug stmts to get discriminators just as
> nondebug stmts would if their line numbers match.
> 
> I suppose we could arrange for discriminators to be omitted from the
> -fcompare-debug dumps, but keeping discriminators in sync is probably
> good to avoid other potential sources of divergence between debug and
> nondebug.
> 
> Regstrapped on x86_64-linux-gnu, also tested with gcc-13 on i686- and
> x86_64-.  Ok to install?
> 
> (Eugene, I suppose what's special about this testcase, that may not
> apply to most other uses of assign_discriminators, is that goacc creates
> new functions out of already optimized code.  I think
> assign_discriminators may not be suitable for new functions, with code
> that isn't exactly pristinely in-order.  WDYT?)
> 
> 
> for  gcc/ChangeLog
> 
> 	* tree-cfg.cc (assign_discriminators): Handle debug stmts.
OK
jeff

RE: [EXTERNAL] [PATCH] skip debug stmts when assigning locus discriminators

[Eugene Rozenfeld]

The fix looks good to me. Will this also fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107169 ? It was also a bad interaction of -gstatement-frontiers and discriminators.

Eugene

-----Original Message-----
From: Alexandre Oliva <oliva@adacore.com>
Sent: Wednesday, November 8, 2023 7:51 AM
To: gcc-patches@gcc.gnu.org
Cc: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>
Subject: [EXTERNAL] [PATCH] skip debug stmts when assigning locus discriminators


c-c++-common/goacc/kernels-loop-g.c has been failing (compare-debug)
on i686-linux-gnu since r13-3172, because the implementation enabled debug stmts to cause discriminators to be assigned differently, and the discriminators are printed in the .gkd dumps that -fcompare-debug compares.

This patch prevents debug stmts from affecting the discriminators in nondebug stmts, but enables debug stmts to get discriminators just as nondebug stmts would if their line numbers match.

I suppose we could arrange for discriminators to be omitted from the -fcompare-debug dumps, but keeping discriminators in sync is probably good to avoid other potential sources of divergence between debug and nondebug.

Regstrapped on x86_64-linux-gnu, also tested with gcc-13 on i686- and x86_64-.  Ok to install?

(Eugene, I suppose what's special about this testcase, that may not apply to most other uses of assign_discriminators, is that goacc creates new functions out of already optimized code.  I think assign_discriminators may not be suitable for new functions, with code that isn't exactly pristinely in-order.  WDYT?)


for  gcc/ChangeLog

        * tree-cfg.cc (assign_discriminators): Handle debug stmts.
---
 gcc/tree-cfg.cc |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index 40a6f2a3b529f..a30a2de33a106 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -1214,6 +1214,22 @@ assign_discriminators (void)
        {
          gimple *stmt = gsi_stmt (gsi);

+         /* Don't allow debug stmts to affect discriminators, but
+            allow them to take discriminators when they're on the
+            same line as the preceding nondebug stmt.  */
+         if (is_gimple_debug (stmt))
+           {
+             if (curr_locus != UNKNOWN_LOCATION
+                 && same_line_p (curr_locus, &curr_locus_e,
+                                 gimple_location (stmt)))
+               {
+                 location_t loc = gimple_location (stmt);
+                 location_t dloc = location_with_discriminator (loc,
+                                                                curr_discr);
+                 gimple_set_location (stmt, dloc);
+               }
+             continue;
+           }
          if (curr_locus == UNKNOWN_LOCATION)
            {
              curr_locus = gimple_location (stmt);

--
Alexandre Oliva, happy hacker            https://fsfla.org/blogs/lxo/
   Free Software Activist                   GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity Excluding neuro-others for not behaving ""normal"" is *not* inclusive