public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
@ 2022-08-23 11:49 Surya Kumari Jangala
  2022-08-23 12:55 ` Peter Bergner
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Surya Kumari Jangala @ 2022-08-23 11:49 UTC (permalink / raw)
  To: gcc-patches, bergner, Segher Boessenkool, pthaugen

sched1: Fix -fcompare-debug issue in schedule_region [PR105586]

In schedule_region(), a basic block that does not contain any real insns
is not scheduled and the dfa state at the entry of the bb is not copied
to the fallthru basic block. However a DEBUG insn is treated as a real
insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
state is copied to the fallthru bb. This was resulting in
-fcompare-debug failure as the incoming dfa state of the fallthru block
is different with -g. We should always copy the dfa state of a bb to
it's fallthru bb even if the bb does not contain real insns.

2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>

gcc/
	PR rtl-optimization/105586
	* sched-rgn.cc (schedule_region): Always copy dfa state to
	fallthru block.

gcc/testsuite/
	PR rtl-optimization/105586
	* gcc.target/powerpc/pr105586.c: New test.


diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc
index 0dc2a8f2851..6c9202c0b2b 100644
--- a/gcc/sched-rgn.cc
+++ b/gcc/sched-rgn.cc
@@ -3082,6 +3082,24 @@ free_bb_state_array (void)
   bb_state = NULL;
 }
 
+static void
+save_state_for_fallthru_edge (basic_block last_bb, state_t state)
+{
+  edge f = find_fallthru_edge (last_bb->succs);
+  if (f
+      && (!f->probability.initialized_p ()
+	  || (f->probability.to_reg_br_prob_base () * 100
+	      / REG_BR_PROB_BASE
+	      >= param_sched_state_edge_prob_cutoff)))
+  {
+    memcpy (bb_state[f->dest->index], state,
+	    dfa_state_size);
+    if (sched_verbose >= 5)
+      fprintf (sched_dump, "saving state for edge %d->%d\n",
+	       f->src->index, f->dest->index);
+  }
+}
+
 /* Schedule a region.  A region is either an inner loop, a loop-free
    subroutine, or a single basic block.  Each bb in the region is
    scheduled after its flow predecessors.  */
@@ -3155,6 +3173,7 @@ schedule_region (int rgn)
       if (no_real_insns_p (head, tail))
 	{
 	  gcc_assert (first_bb == last_bb);
+	  save_state_for_fallthru_edge (last_bb, bb_state[first_bb->index]);
 	  continue;
 	}
 
@@ -3173,26 +3192,13 @@ schedule_region (int rgn)
       curr_bb = first_bb;
       if (dbg_cnt (sched_block))
         {
-	  edge f;
 	  int saved_last_basic_block = last_basic_block_for_fn (cfun);
 
 	  schedule_block (&curr_bb, bb_state[first_bb->index]);
 	  gcc_assert (EBB_FIRST_BB (bb) == first_bb);
 	  sched_rgn_n_insns += sched_n_insns;
 	  realloc_bb_state_array (saved_last_basic_block);
-	  f = find_fallthru_edge (last_bb->succs);
-	  if (f
-	      && (!f->probability.initialized_p ()
-		  || (f->probability.to_reg_br_prob_base () * 100
-		      / REG_BR_PROB_BASE
-		      >= param_sched_state_edge_prob_cutoff)))
-	    {
-	      memcpy (bb_state[f->dest->index], curr_state,
-		      dfa_state_size);
-	      if (sched_verbose >= 5)
-		fprintf (sched_dump, "saving state for edge %d->%d\n",
-			 f->src->index, f->dest->index);
-	    }
+	  save_state_for_fallthru_edge (last_bb, curr_state);
         }
       else
         {
diff --git a/gcc/testsuite/gcc.target/powerpc/pr105586.c b/gcc/testsuite/gcc.target/powerpc/pr105586.c
new file mode 100644
index 00000000000..bd397f58bc0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr105586.c
@@ -0,0 +1,19 @@
+/* { dg-options "-mdejagnu-tune=power4 -O2 -fcompare-debug -fno-if-conversion -fno-guess-branch-probability" } */
+
+extern int bar(int i);
+
+typedef unsigned long u64;
+int g;
+
+__int128 h;
+
+void
+foo(int a, int b) {
+  int i;
+  char u8_1 = g, u8_3 = a;
+  u64 u64_1 = bar(0), u64_3 = u8_3 * u64_1;
+  __int128 u128_1 = h ^ __builtin_expect(i, 0);
+  u64 u64_4 = u64_1 << ((__builtin_sub_overflow_p(0, u8_1, (u64)0) ^ u128_1) & 8);
+  u64 u64_r = b + u64_3 + u64_4;
+  bar((short)u64_r + u8_3);
+}

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-08-23 11:49 [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586] Surya Kumari Jangala
@ 2022-08-23 12:55 ` Peter Bergner
  2022-08-23 13:28   ` Segher Boessenkool
  2022-08-30 12:27 ` Richard Sandiford
  2022-08-31 15:39 ` Jeff Law
  2 siblings, 1 reply; 10+ messages in thread
From: Peter Bergner @ 2022-08-23 12:55 UTC (permalink / raw)
  To: Surya Kumari Jangala, gcc-patches, Segher Boessenkool, pthaugen

On 8/23/22 6:49 AM, Surya Kumari Jangala wrote:
> sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
> 
> In schedule_region(), a basic block that does not contain any real insns
> is not scheduled and the dfa state at the entry of the bb is not copied
> to the fallthru basic block. However a DEBUG insn is treated as a real
> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
> state is copied to the fallthru bb. This was resulting in
> -fcompare-debug failure as the incoming dfa state of the fallthru block
> is different with -g. We should always copy the dfa state of a bb to
> it's fallthru bb even if the bb does not contain real insns.
> 
> 2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>
> 
> gcc/
> 	PR rtl-optimization/105586
> 	* sched-rgn.cc (schedule_region): Always copy dfa state to
> 	fallthru block.


It looks good to me, but I cannot approve it.  That said, you're missing
a ChangeLog entry for your new helper function.  The ChangeLog mentions
what changed, not why it changed.  Maybe something like the following?


gcc/
	PR rtl-optimization/105586
	* sched-rgn.cc (save_state_for_fallthru_edge): New function.
	(schedule_region): Use it for all blocks.


Peter

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-08-23 12:55 ` Peter Bergner
@ 2022-08-23 13:28   ` Segher Boessenkool
  2022-08-24  8:10     ` Surya Kumari Jangala
  0 siblings, 1 reply; 10+ messages in thread
From: Segher Boessenkool @ 2022-08-23 13:28 UTC (permalink / raw)
  To: Peter Bergner; +Cc: Surya Kumari Jangala, gcc-patches, pthaugen

Hi!

On Tue, Aug 23, 2022 at 07:55:22AM -0500, Peter Bergner wrote:
> It looks good to me, but I cannot approve it.

Same here (both statements).

> That said, you're missing
> a ChangeLog entry for your new helper function.  The ChangeLog mentions
> what changed, not why it changed.

And that is correct!  Changelogs should not say that, that isn't their
purpose (in GCC), not what they are used for.  Explanations like that go
in the email and/or the commit message.

The main remaining usefulness of changelogs is to spot unintended
commmits.

> Maybe something like the following?
> 
> gcc/
> 	PR rtl-optimization/105586
> 	* sched-rgn.cc (save_state_for_fallthru_edge): New function.
> 	(schedule_region): Use it for all blocks.

That looks perfect, it doesn't say "why" either :-)


Segher

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-08-23 13:28   ` Segher Boessenkool
@ 2022-08-24  8:10     ` Surya Kumari Jangala
  0 siblings, 0 replies; 10+ messages in thread
From: Surya Kumari Jangala @ 2022-08-24  8:10 UTC (permalink / raw)
  To: Segher Boessenkool, Peter Bergner; +Cc: gcc-patches, pthaugen

Hi Peter, Segher,
Thanks for going thru the patch!
I will make the proposed changes to the Changelog.

Regards,
Surya


On 23/08/22 6:58 pm, Segher Boessenkool wrote:
> Hi!
> 
> On Tue, Aug 23, 2022 at 07:55:22AM -0500, Peter Bergner wrote:
>> It looks good to me, but I cannot approve it.
> 
> Same here (both statements).
> 
>> That said, you're missing
>> a ChangeLog entry for your new helper function.  The ChangeLog mentions
>> what changed, not why it changed.
> 
> And that is correct!  Changelogs should not say that, that isn't their
> purpose (in GCC), not what they are used for.  Explanations like that go
> in the email and/or the commit message.
> 
> The main remaining usefulness of changelogs is to spot unintended
> commmits.
> 
>> Maybe something like the following?
>>
>> gcc/
>> 	PR rtl-optimization/105586
>> 	* sched-rgn.cc (save_state_for_fallthru_edge): New function.
>> 	(schedule_region): Use it for all blocks.
> 
> That looks perfect, it doesn't say "why" either :-)
> 
> 
> Segher

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-08-23 11:49 [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586] Surya Kumari Jangala
  2022-08-23 12:55 ` Peter Bergner
@ 2022-08-30 12:27 ` Richard Sandiford
  2022-08-31 15:39 ` Jeff Law
  2 siblings, 0 replies; 10+ messages in thread
From: Richard Sandiford @ 2022-08-30 12:27 UTC (permalink / raw)
  To: Surya Kumari Jangala via Gcc-patches
  Cc: bergner, Segher Boessenkool, pthaugen, Surya Kumari Jangala

Surya Kumari Jangala via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
>
> In schedule_region(), a basic block that does not contain any real insns
> is not scheduled and the dfa state at the entry of the bb is not copied
> to the fallthru basic block. However a DEBUG insn is treated as a real
> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
> state is copied to the fallthru bb. This was resulting in
> -fcompare-debug failure as the incoming dfa state of the fallthru block
> is different with -g. We should always copy the dfa state of a bb to
> it's fallthru bb even if the bb does not contain real insns.
>
> 2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>
>
> gcc/
> 	PR rtl-optimization/105586
> 	* sched-rgn.cc (schedule_region): Always copy dfa state to
> 	fallthru block.
>
> gcc/testsuite/
> 	PR rtl-optimization/105586
> 	* gcc.target/powerpc/pr105586.c: New test.
>
>
> diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc
> index 0dc2a8f2851..6c9202c0b2b 100644
> --- a/gcc/sched-rgn.cc
> +++ b/gcc/sched-rgn.cc
> @@ -3082,6 +3082,24 @@ free_bb_state_array (void)
>    bb_state = NULL;
>  }
>  
> +static void
> +save_state_for_fallthru_edge (basic_block last_bb, state_t state)

The convention (not always followed) is to have a comment above each
function explaining what it does.  How about something like:

/* If LAST_BB falls through to another block B, record that B should
   start with DFA start STATE.  */

OK for trunk with that change (and with the new changelog).

Thanks,
Richard

> +{
> +  edge f = find_fallthru_edge (last_bb->succs);
> +  if (f
> +      && (!f->probability.initialized_p ()
> +	  || (f->probability.to_reg_br_prob_base () * 100
> +	      / REG_BR_PROB_BASE
> +	      >= param_sched_state_edge_prob_cutoff)))
> +  {
> +    memcpy (bb_state[f->dest->index], state,
> +	    dfa_state_size);
> +    if (sched_verbose >= 5)
> +      fprintf (sched_dump, "saving state for edge %d->%d\n",
> +	       f->src->index, f->dest->index);
> +  }
> +}
> +
>  /* Schedule a region.  A region is either an inner loop, a loop-free
>     subroutine, or a single basic block.  Each bb in the region is
>     scheduled after its flow predecessors.  */
> @@ -3155,6 +3173,7 @@ schedule_region (int rgn)
>        if (no_real_insns_p (head, tail))
>  	{
>  	  gcc_assert (first_bb == last_bb);
> +	  save_state_for_fallthru_edge (last_bb, bb_state[first_bb->index]);
>  	  continue;
>  	}
>  
> @@ -3173,26 +3192,13 @@ schedule_region (int rgn)
>        curr_bb = first_bb;
>        if (dbg_cnt (sched_block))
>          {
> -	  edge f;
>  	  int saved_last_basic_block = last_basic_block_for_fn (cfun);
>  
>  	  schedule_block (&curr_bb, bb_state[first_bb->index]);
>  	  gcc_assert (EBB_FIRST_BB (bb) == first_bb);
>  	  sched_rgn_n_insns += sched_n_insns;
>  	  realloc_bb_state_array (saved_last_basic_block);
> -	  f = find_fallthru_edge (last_bb->succs);
> -	  if (f
> -	      && (!f->probability.initialized_p ()
> -		  || (f->probability.to_reg_br_prob_base () * 100
> -		      / REG_BR_PROB_BASE
> -		      >= param_sched_state_edge_prob_cutoff)))
> -	    {
> -	      memcpy (bb_state[f->dest->index], curr_state,
> -		      dfa_state_size);
> -	      if (sched_verbose >= 5)
> -		fprintf (sched_dump, "saving state for edge %d->%d\n",
> -			 f->src->index, f->dest->index);
> -	    }
> +	  save_state_for_fallthru_edge (last_bb, curr_state);
>          }
>        else
>          {
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr105586.c b/gcc/testsuite/gcc.target/powerpc/pr105586.c
> new file mode 100644
> index 00000000000..bd397f58bc0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr105586.c
> @@ -0,0 +1,19 @@
> +/* { dg-options "-mdejagnu-tune=power4 -O2 -fcompare-debug -fno-if-conversion -fno-guess-branch-probability" } */
> +
> +extern int bar(int i);
> +
> +typedef unsigned long u64;
> +int g;
> +
> +__int128 h;
> +
> +void
> +foo(int a, int b) {
> +  int i;
> +  char u8_1 = g, u8_3 = a;
> +  u64 u64_1 = bar(0), u64_3 = u8_3 * u64_1;
> +  __int128 u128_1 = h ^ __builtin_expect(i, 0);
> +  u64 u64_4 = u64_1 << ((__builtin_sub_overflow_p(0, u8_1, (u64)0) ^ u128_1) & 8);
> +  u64 u64_r = b + u64_3 + u64_4;
> +  bar((short)u64_r + u8_3);
> +}

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-08-23 11:49 [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586] Surya Kumari Jangala
  2022-08-23 12:55 ` Peter Bergner
  2022-08-30 12:27 ` Richard Sandiford
@ 2022-08-31 15:39 ` Jeff Law
  2022-09-20  7:17   ` Surya Kumari Jangala
  2 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2022-08-31 15:39 UTC (permalink / raw)
  To: gcc-patches



On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote:
> sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
>
> In schedule_region(), a basic block that does not contain any real insns
> is not scheduled and the dfa state at the entry of the bb is not copied
> to the fallthru basic block. However a DEBUG insn is treated as a real
> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
> state is copied to the fallthru bb. This was resulting in
> -fcompare-debug failure as the incoming dfa state of the fallthru block
> is different with -g. We should always copy the dfa state of a bb to
> it's fallthru bb even if the bb does not contain real insns.
>
> 2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>
>
> gcc/
> 	PR rtl-optimization/105586
> 	* sched-rgn.cc (schedule_region): Always copy dfa state to
> 	fallthru block.
>
> gcc/testsuite/
> 	PR rtl-optimization/105586
> 	* gcc.target/powerpc/pr105586.c: New test.
Interesting.    We may have stumbled over this bug internally a little 
while ago -- not from a compare-debug standpoint, but from a "why isn't 
the processor state copied to the fallthru block" point of view.   I had 
it on my to investigate list, but hadn't gotten around to it yet.

I think there were requests for ChangeLog updates and a function comment 
for save_state_for_fallthru_edge.  OK with those updates.

jeff


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-08-31 15:39 ` Jeff Law
@ 2022-09-20  7:17   ` Surya Kumari Jangala
  2022-09-21  7:33     ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Surya Kumari Jangala @ 2022-09-20  7:17 UTC (permalink / raw)
  To: Jeff Law, gcc-patches, richard.sandiford

Hi Jeff, Richard,
Thank you for reviewing the patch!
I have committed the patch to the gcc repo.
Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too?

Regards,
Surya


On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote:
> 
> 
> On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote:
>> sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
>>
>> In schedule_region(), a basic block that does not contain any real insns
>> is not scheduled and the dfa state at the entry of the bb is not copied
>> to the fallthru basic block. However a DEBUG insn is treated as a real
>> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
>> state is copied to the fallthru bb. This was resulting in
>> -fcompare-debug failure as the incoming dfa state of the fallthru block
>> is different with -g. We should always copy the dfa state of a bb to
>> it's fallthru bb even if the bb does not contain real insns.
>>
>> 2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>
>>
>> gcc/
>>     PR rtl-optimization/105586
>>     * sched-rgn.cc (schedule_region): Always copy dfa state to
>>     fallthru block.
>>
>> gcc/testsuite/
>>     PR rtl-optimization/105586
>>     * gcc.target/powerpc/pr105586.c: New test.
> Interesting.    We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view.   I had it on my to investigate list, but hadn't gotten around to it yet.
> 
> I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge.  OK with those updates.
> 
> jeff
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-09-20  7:17   ` Surya Kumari Jangala
@ 2022-09-21  7:33     ` Richard Biener
  2022-11-08 16:36       ` Surya Kumari Jangala
  0 siblings, 1 reply; 10+ messages in thread
From: Richard Biener @ 2022-09-21  7:33 UTC (permalink / raw)
  To: Surya Kumari Jangala; +Cc: Jeff Law, GCC Patches, Richard Sandiford

On Tue, Sep 20, 2022 at 9:18 AM Surya Kumari Jangala via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi Jeff, Richard,
> Thank you for reviewing the patch!
> I have committed the patch to the gcc repo.
> Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too?

It doesn't seem to be a regression so I'd error on the safe side here.

Richard.

> Regards,
> Surya
>
>
> On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote:
> >
> >
> > On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote:
> >> sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
> >>
> >> In schedule_region(), a basic block that does not contain any real insns
> >> is not scheduled and the dfa state at the entry of the bb is not copied
> >> to the fallthru basic block. However a DEBUG insn is treated as a real
> >> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
> >> state is copied to the fallthru bb. This was resulting in
> >> -fcompare-debug failure as the incoming dfa state of the fallthru block
> >> is different with -g. We should always copy the dfa state of a bb to
> >> it's fallthru bb even if the bb does not contain real insns.
> >>
> >> 2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>
> >>
> >> gcc/
> >>     PR rtl-optimization/105586
> >>     * sched-rgn.cc (schedule_region): Always copy dfa state to
> >>     fallthru block.
> >>
> >> gcc/testsuite/
> >>     PR rtl-optimization/105586
> >>     * gcc.target/powerpc/pr105586.c: New test.
> > Interesting.    We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view.   I had it on my to investigate list, but hadn't gotten around to it yet.
> >
> > I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge.  OK with those updates.
> >
> > jeff
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-09-21  7:33     ` Richard Biener
@ 2022-11-08 16:36       ` Surya Kumari Jangala
  2022-11-08 17:32         ` Richard Biener
  0 siblings, 1 reply; 10+ messages in thread
From: Surya Kumari Jangala @ 2022-11-08 16:36 UTC (permalink / raw)
  To: Richard Biener; +Cc: Jeff Law, GCC Patches, Richard Sandiford

Hi Richard,

On 21/09/22 1:03 pm, Richard Biener wrote:
> On Tue, Sep 20, 2022 at 9:18 AM Surya Kumari Jangala via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Hi Jeff, Richard,
>> Thank you for reviewing the patch!
>> I have committed the patch to the gcc repo.
>> Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too?
> 
> It doesn't seem to be a regression so I'd error on the safe side here.

Can you please clarify, should this patch be backported? It is not very clear what "safe side" means here.

Thanks!
Surya

> 
> Richard.
> 
>> Regards,
>> Surya
>>
>>
>> On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote:
>>>
>>>
>>> On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote:
>>>> sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
>>>>
>>>> In schedule_region(), a basic block that does not contain any real insns
>>>> is not scheduled and the dfa state at the entry of the bb is not copied
>>>> to the fallthru basic block. However a DEBUG insn is treated as a real
>>>> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
>>>> state is copied to the fallthru bb. This was resulting in
>>>> -fcompare-debug failure as the incoming dfa state of the fallthru block
>>>> is different with -g. We should always copy the dfa state of a bb to
>>>> it's fallthru bb even if the bb does not contain real insns.
>>>>
>>>> 2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>
>>>>
>>>> gcc/
>>>>     PR rtl-optimization/105586
>>>>     * sched-rgn.cc (schedule_region): Always copy dfa state to
>>>>     fallthru block.
>>>>
>>>> gcc/testsuite/
>>>>     PR rtl-optimization/105586
>>>>     * gcc.target/powerpc/pr105586.c: New test.
>>> Interesting.    We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view.   I had it on my to investigate list, but hadn't gotten around to it yet.
>>>
>>> I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge.  OK with those updates.
>>>
>>> jeff
>>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
  2022-11-08 16:36       ` Surya Kumari Jangala
@ 2022-11-08 17:32         ` Richard Biener
  0 siblings, 0 replies; 10+ messages in thread
From: Richard Biener @ 2022-11-08 17:32 UTC (permalink / raw)
  To: Surya Kumari Jangala; +Cc: Jeff Law, GCC Patches, Richard Sandiford

On Tue, Nov 8, 2022 at 5:37 PM Surya Kumari Jangala
<jskumari@linux.vnet.ibm.com> wrote:
>
> Hi Richard,
>
> On 21/09/22 1:03 pm, Richard Biener wrote:
> > On Tue, Sep 20, 2022 at 9:18 AM Surya Kumari Jangala via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> Hi Jeff, Richard,
> >> Thank you for reviewing the patch!
> >> I have committed the patch to the gcc repo.
> >> Can I backport this patch to prior versions of gcc, as this is an easy patch to backport and the issue exists in prior versions too?
> >
> > It doesn't seem to be a regression so I'd error on the safe side here.
>
> Can you please clarify, should this patch be backported? It is not very clear what "safe side" means here.

Not backporting is the safe side.

Richard.

> Thanks!
> Surya
>
> >
> > Richard.
> >
> >> Regards,
> >> Surya
> >>
> >>
> >> On 31/08/22 9:09 pm, Jeff Law via Gcc-patches wrote:
> >>>
> >>>
> >>> On 8/23/2022 5:49 AM, Surya Kumari Jangala via Gcc-patches wrote:
> >>>> sched1: Fix -fcompare-debug issue in schedule_region [PR105586]
> >>>>
> >>>> In schedule_region(), a basic block that does not contain any real insns
> >>>> is not scheduled and the dfa state at the entry of the bb is not copied
> >>>> to the fallthru basic block. However a DEBUG insn is treated as a real
> >>>> insn, and if a bb contains non-real insns and a DEBUG insn, it's dfa
> >>>> state is copied to the fallthru bb. This was resulting in
> >>>> -fcompare-debug failure as the incoming dfa state of the fallthru block
> >>>> is different with -g. We should always copy the dfa state of a bb to
> >>>> it's fallthru bb even if the bb does not contain real insns.
> >>>>
> >>>> 2022-08-22  Surya Kumari Jangala  <jskumari@linux.ibm.com>
> >>>>
> >>>> gcc/
> >>>>     PR rtl-optimization/105586
> >>>>     * sched-rgn.cc (schedule_region): Always copy dfa state to
> >>>>     fallthru block.
> >>>>
> >>>> gcc/testsuite/
> >>>>     PR rtl-optimization/105586
> >>>>     * gcc.target/powerpc/pr105586.c: New test.
> >>> Interesting.    We may have stumbled over this bug internally a little while ago -- not from a compare-debug standpoint, but from a "why isn't the processor state copied to the fallthru block" point of view.   I had it on my to investigate list, but hadn't gotten around to it yet.
> >>>
> >>> I think there were requests for ChangeLog updates and a function comment for save_state_for_fallthru_edge.  OK with those updates.
> >>>
> >>> jeff
> >>>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-11-08 17:33 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-23 11:49 [PATCH] sched1: Fix -fcompare-debug issue in schedule_region [PR105586] Surya Kumari Jangala
2022-08-23 12:55 ` Peter Bergner
2022-08-23 13:28   ` Segher Boessenkool
2022-08-24  8:10     ` Surya Kumari Jangala
2022-08-30 12:27 ` Richard Sandiford
2022-08-31 15:39 ` Jeff Law
2022-09-20  7:17   ` Surya Kumari Jangala
2022-09-21  7:33     ` Richard Biener
2022-11-08 16:36       ` Surya Kumari Jangala
2022-11-08 17:32         ` Richard Biener

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).