public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
@ 2019-11-12  9:16 Matthew Malcomson
  2019-12-02 17:57 ` Matthew Malcomson
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Matthew Malcomson @ 2019-11-12  9:16 UTC (permalink / raw)
  To: gcc-patches; +Cc: rguenther, nd, law, ian, Martin Liska

[-- Attachment #1: Type: text/plain, Size: 4188 bytes --]

In scheduling passes, notes are removed with `remove_notes` before the
scheduling is done, and added back in with `reemit_notes` once the
scheduling has been decided.

This process leaves the notes in the RTL chain with different insn uid's
than were there before.  Having different UID's (larger than the
previous ones) means that DF_INSN_INFO_GET(insn) will access outside of
the allocated array.

This has been seen in the `regstat_bb_compute_calls_crossed` function.
This patch adds an assert to the `regstat_bb_compute_calls_crossed`
function so that bad accesses here are caught instead of going
unnoticed, and then avoids the problem.

We avoid the problem by ensuring that new notes added by `reemit_notes` have an
insn record given to them.  This is done by adding a call to
`df_insn_create_insn_record` on each note added in `reemit_notes`.
`df_insn_create_insn_record` leaves this new record zeroed out, which appears
to be fine for notes (e.g. `df_bb_refs_record` already does not set
anything except the luid for notes, and notes have no dataflow information to
record).

We add the testcase that Martin found here
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92410#c2 .
This testcase fails with the "regstat.c" change, and then succeeds with the
"haifa-sched.c" change.

There is a similar problem with labels, that the `gcc_assert` catches
when running regression tests in gcc.dg/fold-eqandshift-1.c and
gcc.c-torture/compile/pr32482.c.
This is due to the `cfg_layout_finalize` call in `bb-reorder.c` emitting
new labels for the start of the newly created basic blocks. These labels are
not given a dataflow df_insn_info member.

We solve this by manually calling `df_recompute_luids` on each basic
block once this pass has finished.

Testing done:
  Bootstrapped and regression test on aarch64-none-linux-gnu native.

gcc/ChangeLog:

2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>

	PR middle-end/92410
	* bb-reorder.c (pass_reorder_blocks::execute): Recompute
	dataflow luids once basic blocks have been reordered.
	* haifa-sched.c (reemit_notes): Create df insn record for each
	new note.
	* regstat.c (regstat_bb_compute_calls_crossed): Assert every
	insn has an insn record before trying to use it.

gcc/testsuite/ChangeLog:

2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>

	PR middle-end/92410
	* gcc.dg/torture/pr92410.c: New test.



###############     Attachment also inlined for ease of reply    ###############


diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
index 0ac39140c6ce3db8499f99cd8f483218888de61b..4943f97a2cfccc7ab7c4834fff2c81f62fc5b738 100644
--- a/gcc/bb-reorder.c
+++ b/gcc/bb-reorder.c
@@ -2662,6 +2662,8 @@ pass_reorder_blocks::execute (function *fun)
       bb->aux = bb->next_bb;
   cfg_layout_finalize ();
 
+  FOR_EACH_BB_FN (bb, fun)
+    df_recompute_luids (bb);
   return 0;
 }
 
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 41cf1f362e8c34d009b0a310ff5b9a9ffb613631..2e1a84f1325d54ece1cf3c6f0bab607099c71d27 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -5433,6 +5433,7 @@ reemit_notes (rtx_insn *insn)
 
 	  last = emit_note_before (note_type, last);
 	  remove_note (insn, note);
+	  df_insn_create_insn_record (last);
 	}
     }
 }
diff --git a/gcc/regstat.c b/gcc/regstat.c
index 4da9b7cc523cde113df931226ea56310b659e619..c6cefb117d7330cc1b9787c37083e05e2acda2d2 100644
--- a/gcc/regstat.c
+++ b/gcc/regstat.c
@@ -324,6 +324,7 @@ regstat_bb_compute_calls_crossed (unsigned int bb_index, bitmap live)
 
   FOR_BB_INSNS_REVERSE (bb, insn)
     {
+      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
       struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
       unsigned int regno;
 
diff --git a/gcc/testsuite/gcc.dg/torture/pr92410.c b/gcc/testsuite/gcc.dg/torture/pr92410.c
new file mode 100644
index 0000000000000000000000000000000000000000..628e329782260ef36f26506bfbc0d36a93b33f41
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr92410.c
@@ -0,0 +1,8 @@
+/* PR middle-end/92410  */
+/* { dg-do compile } */
+int v;
+
+int a() {
+  ;
+  return v;
+}


[-- Attachment #2: pr92410.patch --]
[-- Type: text/plain, Size: 1608 bytes --]

diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
index 0ac39140c6ce3db8499f99cd8f483218888de61b..4943f97a2cfccc7ab7c4834fff2c81f62fc5b738 100644
--- a/gcc/bb-reorder.c
+++ b/gcc/bb-reorder.c
@@ -2662,6 +2662,8 @@ pass_reorder_blocks::execute (function *fun)
       bb->aux = bb->next_bb;
   cfg_layout_finalize ();
 
+  FOR_EACH_BB_FN (bb, fun)
+    df_recompute_luids (bb);
   return 0;
 }
 
diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
index 41cf1f362e8c34d009b0a310ff5b9a9ffb613631..2e1a84f1325d54ece1cf3c6f0bab607099c71d27 100644
--- a/gcc/haifa-sched.c
+++ b/gcc/haifa-sched.c
@@ -5433,6 +5433,7 @@ reemit_notes (rtx_insn *insn)
 
 	  last = emit_note_before (note_type, last);
 	  remove_note (insn, note);
+	  df_insn_create_insn_record (last);
 	}
     }
 }
diff --git a/gcc/regstat.c b/gcc/regstat.c
index 4da9b7cc523cde113df931226ea56310b659e619..c6cefb117d7330cc1b9787c37083e05e2acda2d2 100644
--- a/gcc/regstat.c
+++ b/gcc/regstat.c
@@ -324,6 +324,7 @@ regstat_bb_compute_calls_crossed (unsigned int bb_index, bitmap live)
 
   FOR_BB_INSNS_REVERSE (bb, insn)
     {
+      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
       struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
       unsigned int regno;
 
diff --git a/gcc/testsuite/gcc.dg/torture/pr92410.c b/gcc/testsuite/gcc.dg/torture/pr92410.c
new file mode 100644
index 0000000000000000000000000000000000000000..628e329782260ef36f26506bfbc0d36a93b33f41
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr92410.c
@@ -0,0 +1,8 @@
+/* PR middle-end/92410  */
+/* { dg-do compile } */
+int v;
+
+int a() {
+  ;
+  return v;
+}


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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2019-11-12  9:16 [mid-end] Add notes to dataflow insn info when re-emitting (PR92410) Matthew Malcomson
@ 2019-12-02 17:57 ` Matthew Malcomson
  2019-12-06 22:47 ` Jeff Law
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Matthew Malcomson @ 2019-12-02 17:57 UTC (permalink / raw)
  To: gcc-patches; +Cc: rguenther, nd, law, ian, Martin Liska

Ping

On 12/11/2019 09:11, Matthew Malcomson wrote:
> In scheduling passes, notes are removed with `remove_notes` before the
> scheduling is done, and added back in with `reemit_notes` once the
> scheduling has been decided.
> 
> This process leaves the notes in the RTL chain with different insn uid's
> than were there before.  Having different UID's (larger than the
> previous ones) means that DF_INSN_INFO_GET(insn) will access outside of
> the allocated array.
> 
> This has been seen in the `regstat_bb_compute_calls_crossed` function.
> This patch adds an assert to the `regstat_bb_compute_calls_crossed`
> function so that bad accesses here are caught instead of going
> unnoticed, and then avoids the problem.
> 
> We avoid the problem by ensuring that new notes added by `reemit_notes` have an
> insn record given to them.  This is done by adding a call to
> `df_insn_create_insn_record` on each note added in `reemit_notes`.
> `df_insn_create_insn_record` leaves this new record zeroed out, which appears
> to be fine for notes (e.g. `df_bb_refs_record` already does not set
> anything except the luid for notes, and notes have no dataflow information to
> record).
> 
> We add the testcase that Martin found here
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92410#c2 .
> This testcase fails with the "regstat.c" change, and then succeeds with the
> "haifa-sched.c" change.
> 
> There is a similar problem with labels, that the `gcc_assert` catches
> when running regression tests in gcc.dg/fold-eqandshift-1.c and
> gcc.c-torture/compile/pr32482.c.
> This is due to the `cfg_layout_finalize` call in `bb-reorder.c` emitting
> new labels for the start of the newly created basic blocks. These labels are
> not given a dataflow df_insn_info member.
> 
> We solve this by manually calling `df_recompute_luids` on each basic
> block once this pass has finished.
> 
> Testing done:
>    Bootstrapped and regression test on aarch64-none-linux-gnu native.
> 
> gcc/ChangeLog:
> 
> 2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>
> 
> 	PR middle-end/92410
> 	* bb-reorder.c (pass_reorder_blocks::execute): Recompute
> 	dataflow luids once basic blocks have been reordered.
> 	* haifa-sched.c (reemit_notes): Create df insn record for each
> 	new note.
> 	* regstat.c (regstat_bb_compute_calls_crossed): Assert every
> 	insn has an insn record before trying to use it.
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>
> 
> 	PR middle-end/92410
> 	* gcc.dg/torture/pr92410.c: New test.
> 
> 
> 
> ###############     Attachment also inlined for ease of reply    ###############
> 
> 
> diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
> index 0ac39140c6ce3db8499f99cd8f483218888de61b..4943f97a2cfccc7ab7c4834fff2c81f62fc5b738 100644
> --- a/gcc/bb-reorder.c
> +++ b/gcc/bb-reorder.c
> @@ -2662,6 +2662,8 @@ pass_reorder_blocks::execute (function *fun)
>         bb->aux = bb->next_bb;
>     cfg_layout_finalize ();
>   
> +  FOR_EACH_BB_FN (bb, fun)
> +    df_recompute_luids (bb);
>     return 0;
>   }
>   
> diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
> index 41cf1f362e8c34d009b0a310ff5b9a9ffb613631..2e1a84f1325d54ece1cf3c6f0bab607099c71d27 100644
> --- a/gcc/haifa-sched.c
> +++ b/gcc/haifa-sched.c
> @@ -5433,6 +5433,7 @@ reemit_notes (rtx_insn *insn)
>   
>   	  last = emit_note_before (note_type, last);
>   	  remove_note (insn, note);
> +	  df_insn_create_insn_record (last);
>   	}
>       }
>   }
> diff --git a/gcc/regstat.c b/gcc/regstat.c
> index 4da9b7cc523cde113df931226ea56310b659e619..c6cefb117d7330cc1b9787c37083e05e2acda2d2 100644
> --- a/gcc/regstat.c
> +++ b/gcc/regstat.c
> @@ -324,6 +324,7 @@ regstat_bb_compute_calls_crossed (unsigned int bb_index, bitmap live)
>   
>     FOR_BB_INSNS_REVERSE (bb, insn)
>       {
> +      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
>         struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
>         unsigned int regno;
>   
> diff --git a/gcc/testsuite/gcc.dg/torture/pr92410.c b/gcc/testsuite/gcc.dg/torture/pr92410.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..628e329782260ef36f26506bfbc0d36a93b33f41
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr92410.c
> @@ -0,0 +1,8 @@
> +/* PR middle-end/92410  */
> +/* { dg-do compile } */
> +int v;
> +
> +int a() {
> +  ;
> +  return v;
> +}
> 

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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2019-11-12  9:16 [mid-end] Add notes to dataflow insn info when re-emitting (PR92410) Matthew Malcomson
  2019-12-02 17:57 ` Matthew Malcomson
@ 2019-12-06 22:47 ` Jeff Law
  2019-12-09 12:48 ` Martin Liška
  2020-02-27  6:55 ` Roman Zhuykov
  3 siblings, 0 replies; 9+ messages in thread
From: Jeff Law @ 2019-12-06 22:47 UTC (permalink / raw)
  To: Matthew Malcomson, gcc-patches; +Cc: rguenther, nd, ian, Martin Liska

On Tue, 2019-11-12 at 09:11 +0000, Matthew Malcomson wrote:
> In scheduling passes, notes are removed with `remove_notes` before
> the
> 
> scheduling is done, and added back in with `reemit_notes` once the
> 
> scheduling has been decided.
> 
> 
> 
> This process leaves the notes in the RTL chain with different insn
> uid's
> 
> than were there before.  Having different UID's (larger than the
> 
> previous ones) means that DF_INSN_INFO_GET(insn) will access outside
> of
> 
> the allocated array.
> 
> 
> 
> This has been seen in the `regstat_bb_compute_calls_crossed`
> function.
> 
> This patch adds an assert to the `regstat_bb_compute_calls_crossed`
> 
> function so that bad accesses here are caught instead of going
> 
> unnoticed, and then avoids the problem.
> 
> 
> 
> We avoid the problem by ensuring that new notes added by
> `reemit_notes` have an
> 
> insn record given to them.  This is done by adding a call to
> 
> `df_insn_create_insn_record` on each note added in `reemit_notes`.
> 
> `df_insn_create_insn_record` leaves this new record zeroed out, which
> appears
> 
> to be fine for notes (e.g. `df_bb_refs_record` already does not set
> 
> anything except the luid for notes, and notes have no dataflow
> information to
> 
> record).
> 
> 
> 
> We add the testcase that Martin found here
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92410#c2 .
> 
> This testcase fails with the "regstat.c" change, and then succeeds
> with the
> 
> "haifa-sched.c" change.
> 
> 
> 
> There is a similar problem with labels, that the `gcc_assert` catches
> 
> when running regression tests in gcc.dg/fold-eqandshift-1.c and
> 
> gcc.c-torture/compile/pr32482.c.
> 
> This is due to the `cfg_layout_finalize` call in `bb-reorder.c`
> emitting
> 
> new labels for the start of the newly created basic blocks. These
> labels are
> 
> not given a dataflow df_insn_info member.
> 
> 
> 
> We solve this by manually calling `df_recompute_luids` on each basic
> 
> block once this pass has finished.
> 
> 
> 
> Testing done:
> 
>   Bootstrapped and regression test on aarch64-none-linux-gnu native.
> 
> 
> 
> gcc/ChangeLog:
> 
> 
> 
> 2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>
> 
> 
> 
>         PR middle-end/92410
> 
>         * bb-reorder.c (pass_reorder_blocks::execute): Recompute
> 
>         dataflow luids once basic blocks have been reordered.
> 
>         * haifa-sched.c (reemit_notes): Create df insn record for
> each
> 
>         new note.
> 
>         * regstat.c (regstat_bb_compute_calls_crossed): Assert every
> 
>         insn has an insn record before trying to use it.
> 
> 
> 
> gcc/testsuite/ChangeLog:
> 
> 
> 
> 2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>
> 
> 
> 
>         PR middle-end/92410
> 
>         * gcc.dg/torture/pr92410.c: New test.
> 
OK
jeff

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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2019-11-12  9:16 [mid-end] Add notes to dataflow insn info when re-emitting (PR92410) Matthew Malcomson
  2019-12-02 17:57 ` Matthew Malcomson
  2019-12-06 22:47 ` Jeff Law
@ 2019-12-09 12:48 ` Martin Liška
  2019-12-09 13:00   ` Matthew Malcomson
                     ` (2 more replies)
  2020-02-27  6:55 ` Roman Zhuykov
  3 siblings, 3 replies; 9+ messages in thread
From: Martin Liška @ 2019-12-09 12:48 UTC (permalink / raw)
  To: Matthew Malcomson, gcc-patches; +Cc: rguenther, nd, law, ian

Hello.

The patch triggers the following warning:

In file included from /home/marxin/Programming/gcc/gcc/regstat.c:23:
/home/marxin/Programming/gcc/gcc/regstat.c: In function ‘void regstat_bb_compute_calls_crossed(unsigned int, bitmap)’:
/home/marxin/Programming/gcc/gcc/regstat.c:327:35: warning: comparison of integer expressions of different signedness: ‘int’ and ‘unsigned int’ [-Wsign-compare]
   327 |       gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
/home/marxin/Programming/gcc/gcc/system.h:748:14: note: in definition of macro ‘gcc_assert’
   748 |    ((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__), 0 : 0))
       |              ^~~~

What about something like:

  gcc/regstat.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/regstat.c b/gcc/regstat.c
index c6cefb117d7..035d48c28ab 100644
--- a/gcc/regstat.c
+++ b/gcc/regstat.c
@@ -324,7 +324,7 @@ regstat_bb_compute_calls_crossed (unsigned int bb_index, bitmap live)
  
    FOR_BB_INSNS_REVERSE (bb, insn)
      {
-      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
+      gcc_assert (INSN_UID (insn) < (int)DF_INSN_SIZE ());
        struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
        unsigned int regno;
  
Martin

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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2019-12-09 12:48 ` Martin Liška
@ 2019-12-09 13:00   ` Matthew Malcomson
  2019-12-09 13:00   ` Matthew Malcomson
  2019-12-09 13:02   ` Segher Boessenkool
  2 siblings, 0 replies; 9+ messages in thread
From: Matthew Malcomson @ 2019-12-09 13:00 UTC (permalink / raw)
  To: Martin Liška, gcc-patches; +Cc: rguenther, nd, law, ian

Ah,  apologies -- you're right.
I'd already committed the patch this morning, so I'll update it with the 
obvious fix.

Thanks for the catch,
Matthew

On 09/12/2019 12:48, Martin Liška wrote:
> Hello.
> 
> The patch triggers the following warning:
> 
> In file included from /home/marxin/Programming/gcc/gcc/regstat.c:23:
> /home/marxin/Programming/gcc/gcc/regstat.c: In function ‘void 
> regstat_bb_compute_calls_crossed(unsigned int, bitmap)’:
> /home/marxin/Programming/gcc/gcc/regstat.c:327:35: warning: comparison 
> of integer expressions of different signedness: ‘int’ and ‘unsigned int’ 
> [-Wsign-compare]
>    327 |       gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
> /home/marxin/Programming/gcc/gcc/system.h:748:14: note: in definition of 
> macro ‘gcc_assert’
>    748 |    ((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, 
> __FUNCTION__), 0 : 0))
>        |              ^~~~
> 
> What about something like:
> 
>   gcc/regstat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/regstat.c b/gcc/regstat.c
> index c6cefb117d7..035d48c28ab 100644
> --- a/gcc/regstat.c
> +++ b/gcc/regstat.c
> @@ -324,7 +324,7 @@ regstat_bb_compute_calls_crossed (unsigned int 
> bb_index, bitmap live)
> 
>     FOR_BB_INSNS_REVERSE (bb, insn)
>       {
> -      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
> +      gcc_assert (INSN_UID (insn) < (int)DF_INSN_SIZE ());
>         struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
>         unsigned int regno;
> 
> Martin

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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2019-12-09 12:48 ` Martin Liška
  2019-12-09 13:00   ` Matthew Malcomson
@ 2019-12-09 13:00   ` Matthew Malcomson
  2019-12-09 13:02   ` Segher Boessenkool
  2 siblings, 0 replies; 9+ messages in thread
From: Matthew Malcomson @ 2019-12-09 13:00 UTC (permalink / raw)
  To: Martin Liška, gcc-patches; +Cc: rguenther, nd, law, ian

Ah,  apologies -- you're right.
I'd already committed the patch this morning, so I'll update it with the 
obvious fix.

Thanks for the catch,
Matthew

On 09/12/2019 12:48, Martin Liška wrote:
> Hello.
> 
> The patch triggers the following warning:
> 
> In file included from /home/marxin/Programming/gcc/gcc/regstat.c:23:
> /home/marxin/Programming/gcc/gcc/regstat.c: In function ‘void 
> regstat_bb_compute_calls_crossed(unsigned int, bitmap)’:
> /home/marxin/Programming/gcc/gcc/regstat.c:327:35: warning: comparison 
> of integer expressions of different signedness: ‘int’ and ‘unsigned int’ 
> [-Wsign-compare]
>    327 |       gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
> /home/marxin/Programming/gcc/gcc/system.h:748:14: note: in definition of 
> macro ‘gcc_assert’
>    748 |    ((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, 
> __FUNCTION__), 0 : 0))
>        |              ^~~~
> 
> What about something like:
> 
>   gcc/regstat.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gcc/regstat.c b/gcc/regstat.c
> index c6cefb117d7..035d48c28ab 100644
> --- a/gcc/regstat.c
> +++ b/gcc/regstat.c
> @@ -324,7 +324,7 @@ regstat_bb_compute_calls_crossed (unsigned int 
> bb_index, bitmap live)
> 
>     FOR_BB_INSNS_REVERSE (bb, insn)
>       {
> -      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
> +      gcc_assert (INSN_UID (insn) < (int)DF_INSN_SIZE ());
>         struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
>         unsigned int regno;
> 
> Martin

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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2019-12-09 12:48 ` Martin Liška
  2019-12-09 13:00   ` Matthew Malcomson
  2019-12-09 13:00   ` Matthew Malcomson
@ 2019-12-09 13:02   ` Segher Boessenkool
  2 siblings, 0 replies; 9+ messages in thread
From: Segher Boessenkool @ 2019-12-09 13:02 UTC (permalink / raw)
  To: Martin Liška; +Cc: Matthew Malcomson, gcc-patches, rguenther, nd, law, ian

On Mon, Dec 09, 2019 at 01:48:51PM +0100, Martin Liška wrote:
> -      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
> +      gcc_assert (INSN_UID (insn) < (int)DF_INSN_SIZE ());

Space after cast please.


Segher

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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2019-11-12  9:16 [mid-end] Add notes to dataflow insn info when re-emitting (PR92410) Matthew Malcomson
                   ` (2 preceding siblings ...)
  2019-12-09 12:48 ` Martin Liška
@ 2020-02-27  6:55 ` Roman Zhuykov
  2020-03-02  9:59   ` Martin Liška
  3 siblings, 1 reply; 9+ messages in thread
From: Roman Zhuykov @ 2020-02-27  6:55 UTC (permalink / raw)
  To: Matthew Malcomson, gcc-patches, Martin Liska; +Cc: rguenther, nd, law, ian

Hi all!

Does anybody considered backporting this?
Given https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92410#c3
"Confirmed also with r247015 (GCC 7 branch point)."

For example, we may apply it without an assertion in release branches.


Roman

12.11.2019 12:11, Matthew Malcomson wrote:
> In scheduling passes, notes are removed with `remove_notes` before the
> scheduling is done, and added back in with `reemit_notes` once the
> scheduling has been decided.
>
> This process leaves the notes in the RTL chain with different insn uid's
> than were there before.  Having different UID's (larger than the
> previous ones) means that DF_INSN_INFO_GET(insn) will access outside of
> the allocated array.
>
> This has been seen in the `regstat_bb_compute_calls_crossed` function.
> This patch adds an assert to the `regstat_bb_compute_calls_crossed`
> function so that bad accesses here are caught instead of going
> unnoticed, and then avoids the problem.
>
> We avoid the problem by ensuring that new notes added by `reemit_notes` have an
> insn record given to them.  This is done by adding a call to
> `df_insn_create_insn_record` on each note added in `reemit_notes`.
> `df_insn_create_insn_record` leaves this new record zeroed out, which appears
> to be fine for notes (e.g. `df_bb_refs_record` already does not set
> anything except the luid for notes, and notes have no dataflow information to
> record).
>
> We add the testcase that Martin found here
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92410#c2 .
> This testcase fails with the "regstat.c" change, and then succeeds with the
> "haifa-sched.c" change.
>
> There is a similar problem with labels, that the `gcc_assert` catches
> when running regression tests in gcc.dg/fold-eqandshift-1.c and
> gcc.c-torture/compile/pr32482.c.
> This is due to the `cfg_layout_finalize` call in `bb-reorder.c` emitting
> new labels for the start of the newly created basic blocks. These labels are
> not given a dataflow df_insn_info member.
>
> We solve this by manually calling `df_recompute_luids` on each basic
> block once this pass has finished.
>
> Testing done:
>   Bootstrapped and regression test on aarch64-none-linux-gnu native.
>
> gcc/ChangeLog:
>
> 2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>
>
> 	PR middle-end/92410
> 	* bb-reorder.c (pass_reorder_blocks::execute): Recompute
> 	dataflow luids once basic blocks have been reordered.
> 	* haifa-sched.c (reemit_notes): Create df insn record for each
> 	new note.
> 	* regstat.c (regstat_bb_compute_calls_crossed): Assert every
> 	insn has an insn record before trying to use it.
>
> gcc/testsuite/ChangeLog:
>
> 2019-11-12  Matthew Malcomson  <matthew.malcomson@arm.com>
>
> 	PR middle-end/92410
> 	* gcc.dg/torture/pr92410.c: New test.
>
>
>
> ###############     Attachment also inlined for ease of reply    ###############
>
>
> diff --git a/gcc/bb-reorder.c b/gcc/bb-reorder.c
> index 0ac39140c6ce3db8499f99cd8f483218888de61b..4943f97a2cfccc7ab7c4834fff2c81f62fc5b738 100644
> --- a/gcc/bb-reorder.c
> +++ b/gcc/bb-reorder.c
> @@ -2662,6 +2662,8 @@ pass_reorder_blocks::execute (function *fun)
>        bb->aux = bb->next_bb;
>    cfg_layout_finalize ();
>  
> +  FOR_EACH_BB_FN (bb, fun)
> +    df_recompute_luids (bb);
>    return 0;
>  }
>  
> diff --git a/gcc/haifa-sched.c b/gcc/haifa-sched.c
> index 41cf1f362e8c34d009b0a310ff5b9a9ffb613631..2e1a84f1325d54ece1cf3c6f0bab607099c71d27 100644
> --- a/gcc/haifa-sched.c
> +++ b/gcc/haifa-sched.c
> @@ -5433,6 +5433,7 @@ reemit_notes (rtx_insn *insn)
>  
>  	  last = emit_note_before (note_type, last);
>  	  remove_note (insn, note);
> +	  df_insn_create_insn_record (last);
>  	}
>      }
>  }
> diff --git a/gcc/regstat.c b/gcc/regstat.c
> index 4da9b7cc523cde113df931226ea56310b659e619..c6cefb117d7330cc1b9787c37083e05e2acda2d2 100644
> --- a/gcc/regstat.c
> +++ b/gcc/regstat.c
> @@ -324,6 +324,7 @@ regstat_bb_compute_calls_crossed (unsigned int bb_index, bitmap live)
>  
>    FOR_BB_INSNS_REVERSE (bb, insn)
>      {
> +      gcc_assert (INSN_UID (insn) < DF_INSN_SIZE ());
>        struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn);
>        unsigned int regno;
>  
> diff --git a/gcc/testsuite/gcc.dg/torture/pr92410.c b/gcc/testsuite/gcc.dg/torture/pr92410.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..628e329782260ef36f26506bfbc0d36a93b33f41
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr92410.c
> @@ -0,0 +1,8 @@
> +/* PR middle-end/92410  */
> +/* { dg-do compile } */
> +int v;
> +
> +int a() {
> +  ;
> +  return v;
> +}
>

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

* Re: [mid-end] Add notes to dataflow insn info when re-emitting (PR92410)
  2020-02-27  6:55 ` Roman Zhuykov
@ 2020-03-02  9:59   ` Martin Liška
  0 siblings, 0 replies; 9+ messages in thread
From: Martin Liška @ 2020-03-02  9:59 UTC (permalink / raw)
  To: Roman Zhuykov, Matthew Malcomson, gcc-patches
  Cc: rguenther, nd, law, ian, Jakub Jelinek

On 2/27/20 7:55 AM, Roman Zhuykov wrote:
> Does anybody considered backporting this?

I'm for the backport (after 8.4.0 will be released).
I would like to get a permission from release managers about this patch?

Martin

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

end of thread, other threads:[~2020-03-02  9:59 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12  9:16 [mid-end] Add notes to dataflow insn info when re-emitting (PR92410) Matthew Malcomson
2019-12-02 17:57 ` Matthew Malcomson
2019-12-06 22:47 ` Jeff Law
2019-12-09 12:48 ` Martin Liška
2019-12-09 13:00   ` Matthew Malcomson
2019-12-09 13:00   ` Matthew Malcomson
2019-12-09 13:02   ` Segher Boessenkool
2020-02-27  6:55 ` Roman Zhuykov
2020-03-02  9:59   ` Martin Liška

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