| Message ID | patch-18247-tamar@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end: fix ICE when moving statements to empty BB [PR113731] | expand |
On Mon, 5 Feb 2024, Tamar Christina wrote: > Hi All, > > We use gsi_move_before (&stmt_gsi, &dest_gsi); to request that the new statement > be placed before any other statement. Typically this then moves the current > pointer to be after the statement we just inserted. > > However it looks like when the BB is empty, this does not happen and the CUR > pointer stays NULL. There's a comment in the source of gsi_insert_before that > explains: > > /* If CUR is NULL, we link at the end of the sequence (this case happens > > so it adds it to the end instead of start like you asked. This means that in > this case there's nothing to move and so we shouldn't move the pointer if we're > already at the HEAD. The issue is that a gsi_end_p () is ambiguous, it could be the start or the end. gsi_insert_before treats it as "end" while gsi_insert_after treats it as "start" since you can't really insert "after" the "end". gsi_move_before doesn't update the insertion pointer (using GSI_SAME_STMT), so with a gsi_end_p () you get what you ask for. Btw, /* Move all stmts that need moving. */ basic_block dest_bb = LOOP_VINFO_EARLY_BRK_DEST_BB (loop_vinfo); gimple_stmt_iterator dest_gsi = gsi_start_bb (dest_bb); should probably use gsi_after_labels (dest_bb) just in case. It looks like LOOP_VINFO_EARLY_BRK_STORES is "reverse"? Is that why you are doing gsi_move_before + gsi_prev? Why do gsi_prev at all? > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > PR tree-optimization/113731 > * tree-vect-loop.cc (move_early_exit_stmts): Conditionally move pointer. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/113731 > * gcc.dg/vect/vect-early-break_111-pr113731.c: New test. > > --- inline copy of patch -- > diff --git a/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c > new file mode 100644 > index 0000000000000000000000000000000000000000..2d6db91df97625a7f11609d034e89af0461129b2 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c > @@ -0,0 +1,21 @@ > +/* { dg-do compile } */ > +/* { dg-add-options vect_early_break } */ > +/* { dg-require-effective-target vect_early_break } */ > +/* { dg-require-effective-target vect_int } */ > + > +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ > + > +char* inet_net_pton_ipv4_bits; > +char inet_net_pton_ipv4_odst; > +void __errno_location(); > +void inet_net_pton_ipv4(); > +void inet_net_pton() { inet_net_pton_ipv4(); } > +void inet_net_pton_ipv4(char *dst, int size) { > + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { > + if (size-- <= 0) > + goto emsgsize; > + *dst++ = '\0'; > + } > +emsgsize: > + __errno_location(); > +} > diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc > index 30b90d99925bea74caf14833d8ab1695607d0fe9..e2587315020a35a7d4ebd3e7a9842caa36bb5d3c 100644 > --- a/gcc/tree-vect-loop.cc > +++ b/gcc/tree-vect-loop.cc > @@ -11801,7 +11801,8 @@ move_early_exit_stmts (loop_vec_info loop_vinfo) > > gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); > gsi_move_before (&stmt_gsi, &dest_gsi); > - gsi_prev (&dest_gsi); > + if (!gsi_end_p (dest_gsi)) > + gsi_prev (&dest_gsi); > } > > /* Update all the stmts with their new reaching VUSES. */ > > > > >
> -----Original Message----- > From: Richard Biener <rguenther@suse.de> > Sent: Monday, February 5, 2024 1:22 PM > To: Tamar Christina <Tamar.Christina@arm.com> > Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; jlaw@ventanamicro.com > Subject: Re: [PATCH]middle-end: fix ICE when moving statements to empty BB > [PR113731] > > On Mon, 5 Feb 2024, Tamar Christina wrote: > > > Hi All, > > > > We use gsi_move_before (&stmt_gsi, &dest_gsi); to request that the new > statement > > be placed before any other statement. Typically this then moves the current > > pointer to be after the statement we just inserted. > > > > However it looks like when the BB is empty, this does not happen and the CUR > > pointer stays NULL. There's a comment in the source of gsi_insert_before that > > explains: > > > > /* If CUR is NULL, we link at the end of the sequence (this case happens > > > > so it adds it to the end instead of start like you asked. This means that in > > this case there's nothing to move and so we shouldn't move the pointer if we're > > already at the HEAD. > > The issue is that a gsi_end_p () is ambiguous, it could be the start > or the end. gsi_insert_before treats it as "end" while gsi_insert_after > treats it as "start" since you can't really insert "after" the "end". > > gsi_move_before doesn't update the insertion pointer (using > GSI_SAME_STMT), so with a gsi_end_p () you get what you ask for. > > Btw, > > /* Move all stmts that need moving. */ > basic_block dest_bb = LOOP_VINFO_EARLY_BRK_DEST_BB (loop_vinfo); > gimple_stmt_iterator dest_gsi = gsi_start_bb (dest_bb); > > should probably use gsi_after_labels (dest_bb) just in case. See next patch. > > It looks like LOOP_VINFO_EARLY_BRK_STORES is "reverse"? Is that > why you are doing gsi_move_before + gsi_prev? Why do gsi_prev > at all? > Yes, it stores them reverse because we record them from the latch on up. So we either have to iterate backwards, insert them to the front or move gsi. I guess I could remove it by removing the for-each loop and iterating in reverse. Is that preferred? Tamar. > > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. > > > > Ok for master? > > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > PR tree-optimization/113731 > > * tree-vect-loop.cc (move_early_exit_stmts): Conditionally move pointer. > > > > gcc/testsuite/ChangeLog: > > > > PR tree-optimization/113731 > > * gcc.dg/vect/vect-early-break_111-pr113731.c: New test. > > > > --- inline copy of patch -- > > diff --git a/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c > b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c > > new file mode 100644 > > index > 0000000000000000000000000000000000000000..2d6db91df97625a7f1160 > 9d034e89af0461129b2 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c > > @@ -0,0 +1,21 @@ > > +/* { dg-do compile } */ > > +/* { dg-add-options vect_early_break } */ > > +/* { dg-require-effective-target vect_early_break } */ > > +/* { dg-require-effective-target vect_int } */ > > + > > +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ > > + > > +char* inet_net_pton_ipv4_bits; > > +char inet_net_pton_ipv4_odst; > > +void __errno_location(); > > +void inet_net_pton_ipv4(); > > +void inet_net_pton() { inet_net_pton_ipv4(); } > > +void inet_net_pton_ipv4(char *dst, int size) { > > + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { > > + if (size-- <= 0) > > + goto emsgsize; > > + *dst++ = '\0'; > > + } > > +emsgsize: > > + __errno_location(); > > +} > > diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc > > index > 30b90d99925bea74caf14833d8ab1695607d0fe9..e2587315020a35a7d4ebd3e > 7a9842caa36bb5d3c 100644 > > --- a/gcc/tree-vect-loop.cc > > +++ b/gcc/tree-vect-loop.cc > > @@ -11801,7 +11801,8 @@ move_early_exit_stmts (loop_vec_info loop_vinfo) > > > > gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); > > gsi_move_before (&stmt_gsi, &dest_gsi); > > - gsi_prev (&dest_gsi); > > + if (!gsi_end_p (dest_gsi)) > > + gsi_prev (&dest_gsi); > > } > > > > /* Update all the stmts with their new reaching VUSES. */ > > > > > > > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, > Frankenstrasse 146, 90461 Nuernberg, Germany; > GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
> It looks like LOOP_VINFO_EARLY_BRK_STORES is "reverse"? Is that > why you are doing gsi_move_before + gsi_prev? Why do gsi_prev > at all? > As discussed on IRC, then how about this one. Incremental building passed all tests and bootstrap is running. Ok for master if bootstrap and regtesting clean? Thanks, Tamar gcc/ChangeLog: PR tree-optimization/113731 * gimple-iterator.cc (gsi_move_before): Take new parameter for update method. * gimple-iterator.h (gsi_move_before): Default new param to GSI_SAME_STMT. * tree-vect-loop.cc (move_early_exit_stmts): Call gsi_move_before with GSI_NEW_STMT. gcc/testsuite/ChangeLog: PR tree-optimization/113731 * gcc.dg/vect/vect-early-break_111-pr113731.c: New test. --- inline copy of patch --- diff --git a/gcc/gimple-iterator.cc b/gcc/gimple-iterator.cc index 517c53376f0511af59e124f52ec7be566a6c4789..f67bcfbfdfdd7c6cb0ad0130972f5b1dc4429bcf 100644 --- a/gcc/gimple-iterator.cc +++ b/gcc/gimple-iterator.cc @@ -666,10 +666,11 @@ gsi_move_after (gimple_stmt_iterator *from, gimple_stmt_iterator *to) /* Move the statement at FROM so it comes right before the statement - at TO. */ + at TO using method M. */ void -gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to) +gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to, + gsi_iterator_update m = GSI_SAME_STMT) { gimple *stmt = gsi_stmt (*from); gsi_remove (from, false); @@ -677,7 +678,7 @@ gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to) /* For consistency with gsi_move_after, it might be better to have GSI_NEW_STMT here; however, that breaks several places that expect that TO does not change. */ - gsi_insert_before (to, stmt, GSI_SAME_STMT); + gsi_insert_before (to, stmt, m); } diff --git a/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c new file mode 100644 index 0000000000000000000000000000000000000000..2d6db91df97625a7f11609d034e89af0461129b2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-add-options vect_early_break } */ +/* { dg-require-effective-target vect_early_break } */ +/* { dg-require-effective-target vect_int } */ + +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ + +char* inet_net_pton_ipv4_bits; +char inet_net_pton_ipv4_odst; +void __errno_location(); +void inet_net_pton_ipv4(); +void inet_net_pton() { inet_net_pton_ipv4(); } +void inet_net_pton_ipv4(char *dst, int size) { + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { + if (size-- <= 0) + goto emsgsize; + *dst++ = '\0'; + } +emsgsize: + __errno_location(); +} diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc index 30b90d99925bea74caf14833d8ab1695607d0fe9..9aba94bd6ca2061a19487ac4a2735a16d03bcbee 100644 --- a/gcc/tree-vect-loop.cc +++ b/gcc/tree-vect-loop.cc @@ -11800,8 +11800,7 @@ move_early_exit_stmts (loop_vec_info loop_vinfo) dump_printf_loc (MSG_NOTE, vect_location, "moving stmt %G", stmt); gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); - gsi_move_before (&stmt_gsi, &dest_gsi); - gsi_prev (&dest_gsi); + gsi_move_before (&stmt_gsi, &dest_gsi, GSI_NEW_STMT); } /* Update all the stmts with their new reaching VUSES. */
On Mon, 5 Feb 2024, Tamar Christina wrote: > > It looks like LOOP_VINFO_EARLY_BRK_STORES is "reverse"? Is that > > why you are doing gsi_move_before + gsi_prev? Why do gsi_prev > > at all? > > > > As discussed on IRC, then how about this one. > Incremental building passed all tests and bootstrap is running. > > Ok for master if bootstrap and regtesting clean? > > Thanks, > Tamar > > gcc/ChangeLog: > > PR tree-optimization/113731 > * gimple-iterator.cc (gsi_move_before): Take new parameter for update > method. > * gimple-iterator.h (gsi_move_before): Default new param to > GSI_SAME_STMT. > * tree-vect-loop.cc (move_early_exit_stmts): Call gsi_move_before with > GSI_NEW_STMT. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/113731 > * gcc.dg/vect/vect-early-break_111-pr113731.c: New test. > > --- inline copy of patch --- > > diff --git a/gcc/gimple-iterator.cc b/gcc/gimple-iterator.cc > index 517c53376f0511af59e124f52ec7be566a6c4789..f67bcfbfdfdd7c6cb0ad0130972f5b1dc4429bcf 100644 > --- a/gcc/gimple-iterator.cc > +++ b/gcc/gimple-iterator.cc > @@ -666,10 +666,11 @@ gsi_move_after (gimple_stmt_iterator *from, gimple_stmt_iterator *to) > > > /* Move the statement at FROM so it comes right before the statement > - at TO. */ > + at TO using method M. */ > > void > -gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to) > +gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to, > + gsi_iterator_update m = GSI_SAME_STMT) Looks like the wrong patch attached? This should be like in the ChangeLog. OK with that change Richard. > { > gimple *stmt = gsi_stmt (*from); > gsi_remove (from, false); > @@ -677,7 +678,7 @@ gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to) > /* For consistency with gsi_move_after, it might be better to have > GSI_NEW_STMT here; however, that breaks several places that expect > that TO does not change. */ > - gsi_insert_before (to, stmt, GSI_SAME_STMT); > + gsi_insert_before (to, stmt, m); > } > > > diff --git a/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c > new file mode 100644 > index 0000000000000000000000000000000000000000..2d6db91df97625a7f11609d034e89af0461129b2 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c > @@ -0,0 +1,21 @@ > +/* { dg-do compile } */ > +/* { dg-add-options vect_early_break } */ > +/* { dg-require-effective-target vect_early_break } */ > +/* { dg-require-effective-target vect_int } */ > + > +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ > + > +char* inet_net_pton_ipv4_bits; > +char inet_net_pton_ipv4_odst; > +void __errno_location(); > +void inet_net_pton_ipv4(); > +void inet_net_pton() { inet_net_pton_ipv4(); } > +void inet_net_pton_ipv4(char *dst, int size) { > + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { > + if (size-- <= 0) > + goto emsgsize; > + *dst++ = '\0'; > + } > +emsgsize: > + __errno_location(); > +} > diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc > index 30b90d99925bea74caf14833d8ab1695607d0fe9..9aba94bd6ca2061a19487ac4a2735a16d03bcbee 100644 > --- a/gcc/tree-vect-loop.cc > +++ b/gcc/tree-vect-loop.cc > @@ -11800,8 +11800,7 @@ move_early_exit_stmts (loop_vec_info loop_vinfo) > dump_printf_loc (MSG_NOTE, vect_location, "moving stmt %G", stmt); > > gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); > - gsi_move_before (&stmt_gsi, &dest_gsi); > - gsi_prev (&dest_gsi); > + gsi_move_before (&stmt_gsi, &dest_gsi, GSI_NEW_STMT); > } > > /* Update all the stmts with their new reaching VUSES. */ >
On Mon, Feb 05, 2024 at 03:31:20PM +0100, Richard Biener wrote: > On Mon, 5 Feb 2024, Tamar Christina wrote: > > > It looks like LOOP_VINFO_EARLY_BRK_STORES is "reverse"? Is that > > > why you are doing gsi_move_before + gsi_prev? Why do gsi_prev > > > at all? > > > > > > > As discussed on IRC, then how about this one. > > Incremental building passed all tests and bootstrap is running. > > > > Ok for master if bootstrap and regtesting clean? > > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > PR tree-optimization/113731 > > * gimple-iterator.cc (gsi_move_before): Take new parameter for update > > method. > > * gimple-iterator.h (gsi_move_before): Default new param to > > GSI_SAME_STMT. > > * tree-vect-loop.cc (move_early_exit_stmts): Call gsi_move_before with > > GSI_NEW_STMT. > > > > gcc/testsuite/ChangeLog: > > > > PR tree-optimization/113731 > > * gcc.dg/vect/vect-early-break_111-pr113731.c: New test. So like following (Tobias asked for it on IRC)? Seems it FAILs for me on x86_64-linux unless using -msse4.2 (or -msse4), dg-add-options vect_early_break adds just -msse4.1. I believe the difference is in presence/absence of the PCMPGTQ instruction, the loop is comparing pointers (other than equality comparison) and so needs it, while in SSE4.1 one only has PCMPGT{B,W,D}, so it can > compare 16-byte vectors with 8-bit, 16-bit or 32-bit elements. Shall we add /* { dg-additional-options "-msse4.2" { target i?86-*-* x86_64-*-* } } */ to the testcase? --- gcc/gimple-iterator.h.jj 2024-01-03 11:51:33.090709553 +0100 +++ gcc/gimple-iterator.h 2024-02-06 15:23:40.732532207 +0100 @@ -86,7 +86,8 @@ extern gimple_stmt_iterator gsi_for_stmt extern gimple_stmt_iterator gsi_for_stmt (gimple *, gimple_seq *); extern gphi_iterator gsi_for_phi (gphi *); extern void gsi_move_after (gimple_stmt_iterator *, gimple_stmt_iterator *); -extern void gsi_move_before (gimple_stmt_iterator *, gimple_stmt_iterator *); +extern void gsi_move_before (gimple_stmt_iterator *, gimple_stmt_iterator *, + gsi_iterator_update = GSI_SAME_STMT); extern void gsi_move_to_bb_end (gimple_stmt_iterator *, basic_block); extern void gsi_insert_on_edge (edge, gimple *); extern void gsi_insert_seq_on_edge (edge, gimple_seq); --- gcc/gimple-iterator.cc.jj 2024-01-03 11:51:42.016585669 +0100 +++ gcc/gimple-iterator.cc 2024-02-06 15:23:12.274926647 +0100 @@ -666,10 +666,11 @@ gsi_move_after (gimple_stmt_iterator *fr /* Move the statement at FROM so it comes right before the statement - at TO. */ + at TO using method M. */ void -gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to) +gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to, + gsi_iterator_update m) { gimple *stmt = gsi_stmt (*from); gsi_remove (from, false); @@ -677,7 +678,7 @@ gsi_move_before (gimple_stmt_iterator *f /* For consistency with gsi_move_after, it might be better to have GSI_NEW_STMT here; however, that breaks several places that expect that TO does not change. */ - gsi_insert_before (to, stmt, GSI_SAME_STMT); + gsi_insert_before (to, stmt, m); } --- gcc/tree-vect-loop.cc.jj 2024-01-25 09:06:34.116831262 +0100 +++ gcc/tree-vect-loop.cc 2024-02-06 15:22:49.268245536 +0100 @@ -11800,8 +11800,7 @@ move_early_exit_stmts (loop_vec_info loo dump_printf_loc (MSG_NOTE, vect_location, "moving stmt %G", stmt); gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); - gsi_move_before (&stmt_gsi, &dest_gsi); - gsi_prev (&dest_gsi); + gsi_move_before (&stmt_gsi, &dest_gsi, GSI_NEW_STMT); } /* Update all the stmts with their new reaching VUSES. */ --- gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c.jj 2024-02-06 15:22:49.248245813 +0100 +++ gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c 2024-02-06 15:22:49.248245813 +0100 @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-add-options vect_early_break } */ +/* { dg-require-effective-target vect_early_break } */ +/* { dg-require-effective-target vect_int } */ + +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ + +char* inet_net_pton_ipv4_bits; +char inet_net_pton_ipv4_odst; +void __errno_location(); +void inet_net_pton_ipv4(); +void inet_net_pton() { inet_net_pton_ipv4(); } +void inet_net_pton_ipv4(char *dst, int size) { + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { + if (size-- <= 0) + goto emsgsize; + *dst++ = '\0'; + } +emsgsize: + __errno_location(); +} Jakub
On Tue, 6 Feb 2024, Jakub Jelinek wrote: > On Mon, Feb 05, 2024 at 03:31:20PM +0100, Richard Biener wrote: > > On Mon, 5 Feb 2024, Tamar Christina wrote: > > > > It looks like LOOP_VINFO_EARLY_BRK_STORES is "reverse"? Is that > > > > why you are doing gsi_move_before + gsi_prev? Why do gsi_prev > > > > at all? > > > > > > > > > > As discussed on IRC, then how about this one. > > > Incremental building passed all tests and bootstrap is running. > > > > > > Ok for master if bootstrap and regtesting clean? > > > > > > Thanks, > > > Tamar > > > > > > gcc/ChangeLog: > > > > > > PR tree-optimization/113731 > > > * gimple-iterator.cc (gsi_move_before): Take new parameter for update > > > method. > > > * gimple-iterator.h (gsi_move_before): Default new param to > > > GSI_SAME_STMT. > > > * tree-vect-loop.cc (move_early_exit_stmts): Call gsi_move_before with > > > GSI_NEW_STMT. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR tree-optimization/113731 > > > * gcc.dg/vect/vect-early-break_111-pr113731.c: New test. > > So like following (Tobias asked for it on IRC)? > Seems it FAILs for me on x86_64-linux unless using -msse4.2 (or -msse4), > dg-add-options vect_early_break adds just -msse4.1. > I believe the difference is in presence/absence of the PCMPGTQ instruction, > the loop is comparing pointers (other than equality comparison) and so needs > it, while in SSE4.1 one only has PCMPGT{B,W,D}, so it can > compare 16-byte > vectors with 8-bit, 16-bit or 32-bit elements. > > Shall we add > /* { dg-additional-options "-msse4.2" { target i?86-*-* x86_64-*-* } } */ > to the testcase? or add vect_long and/or vect_early_break_long? > --- gcc/gimple-iterator.h.jj 2024-01-03 11:51:33.090709553 +0100 > +++ gcc/gimple-iterator.h 2024-02-06 15:23:40.732532207 +0100 > @@ -86,7 +86,8 @@ extern gimple_stmt_iterator gsi_for_stmt > extern gimple_stmt_iterator gsi_for_stmt (gimple *, gimple_seq *); > extern gphi_iterator gsi_for_phi (gphi *); > extern void gsi_move_after (gimple_stmt_iterator *, gimple_stmt_iterator *); > -extern void gsi_move_before (gimple_stmt_iterator *, gimple_stmt_iterator *); > +extern void gsi_move_before (gimple_stmt_iterator *, gimple_stmt_iterator *, > + gsi_iterator_update = GSI_SAME_STMT); > extern void gsi_move_to_bb_end (gimple_stmt_iterator *, basic_block); > extern void gsi_insert_on_edge (edge, gimple *); > extern void gsi_insert_seq_on_edge (edge, gimple_seq); > --- gcc/gimple-iterator.cc.jj 2024-01-03 11:51:42.016585669 +0100 > +++ gcc/gimple-iterator.cc 2024-02-06 15:23:12.274926647 +0100 > @@ -666,10 +666,11 @@ gsi_move_after (gimple_stmt_iterator *fr > > > /* Move the statement at FROM so it comes right before the statement > - at TO. */ > + at TO using method M. */ > > void > -gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to) > +gsi_move_before (gimple_stmt_iterator *from, gimple_stmt_iterator *to, > + gsi_iterator_update m) > { > gimple *stmt = gsi_stmt (*from); > gsi_remove (from, false); > @@ -677,7 +678,7 @@ gsi_move_before (gimple_stmt_iterator *f > /* For consistency with gsi_move_after, it might be better to have > GSI_NEW_STMT here; however, that breaks several places that expect > that TO does not change. */ > - gsi_insert_before (to, stmt, GSI_SAME_STMT); > + gsi_insert_before (to, stmt, m); > } > > > --- gcc/tree-vect-loop.cc.jj 2024-01-25 09:06:34.116831262 +0100 > +++ gcc/tree-vect-loop.cc 2024-02-06 15:22:49.268245536 +0100 > @@ -11800,8 +11800,7 @@ move_early_exit_stmts (loop_vec_info loo > dump_printf_loc (MSG_NOTE, vect_location, "moving stmt %G", stmt); > > gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); > - gsi_move_before (&stmt_gsi, &dest_gsi); > - gsi_prev (&dest_gsi); > + gsi_move_before (&stmt_gsi, &dest_gsi, GSI_NEW_STMT); > } > > /* Update all the stmts with their new reaching VUSES. */ > --- gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c.jj 2024-02-06 15:22:49.248245813 +0100 > +++ gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c 2024-02-06 15:22:49.248245813 +0100 > @@ -0,0 +1,21 @@ > +/* { dg-do compile } */ > +/* { dg-add-options vect_early_break } */ > +/* { dg-require-effective-target vect_early_break } */ > +/* { dg-require-effective-target vect_int } */ > + > +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ > + > +char* inet_net_pton_ipv4_bits; > +char inet_net_pton_ipv4_odst; > +void __errno_location(); > +void inet_net_pton_ipv4(); > +void inet_net_pton() { inet_net_pton_ipv4(); } > +void inet_net_pton_ipv4(char *dst, int size) { > + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { > + if (size-- <= 0) > + goto emsgsize; > + *dst++ = '\0'; > + } > +emsgsize: > + __errno_location(); > +} > > > Jakub > >
--- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-add-options vect_early_break } */ +/* { dg-require-effective-target vect_early_break } */ +/* { dg-require-effective-target vect_int } */ + +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ + +char* inet_net_pton_ipv4_bits; +char inet_net_pton_ipv4_odst; +void __errno_location(); +void inet_net_pton_ipv4(); +void inet_net_pton() { inet_net_pton_ipv4(); } +void inet_net_pton_ipv4(char *dst, int size) { + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { + if (size-- <= 0) + goto emsgsize; + *dst++ = '\0'; + } +emsgsize: + __errno_location(); +} diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc index 30b90d99925bea74caf14833d8ab1695607d0fe9..e2587315020a35a7d4ebd3e7a9842caa36bb5d3c 100644 --- a/gcc/tree-vect-loop.cc +++ b/gcc/tree-vect-loop.cc @@ -11801,7 +11801,8 @@ move_early_exit_stmts (loop_vec_info loop_vinfo) gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); gsi_move_before (&stmt_gsi, &dest_gsi); - gsi_prev (&dest_gsi); + if (!gsi_end_p (dest_gsi)) + gsi_prev (&dest_gsi); } /* Update all the stmts with their new reaching VUSES. */
Hi All, We use gsi_move_before (&stmt_gsi, &dest_gsi); to request that the new statement be placed before any other statement. Typically this then moves the current pointer to be after the statement we just inserted. However it looks like when the BB is empty, this does not happen and the CUR pointer stays NULL. There's a comment in the source of gsi_insert_before that explains: /* If CUR is NULL, we link at the end of the sequence (this case happens so it adds it to the end instead of start like you asked. This means that in this case there's nothing to move and so we shouldn't move the pointer if we're already at the HEAD. Bootstrapped Regtested on aarch64-none-linux-gnu and no issues. Ok for master? Thanks, Tamar gcc/ChangeLog: PR tree-optimization/113731 * tree-vect-loop.cc (move_early_exit_stmts): Conditionally move pointer. gcc/testsuite/ChangeLog: PR tree-optimization/113731 * gcc.dg/vect/vect-early-break_111-pr113731.c: New test. --- inline copy of patch -- diff --git a/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c new file mode 100644 index 0000000000000000000000000000000000000000..2d6db91df97625a7f11609d034e89af0461129b2 -- diff --git a/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c new file mode 100644 index 0000000000000000000000000000000000000000..2d6db91df97625a7f11609d034e89af0461129b2 --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/vect-early-break_111-pr113731.c @@ -0,0 +1,21 @@ +/* { dg-do compile } */ +/* { dg-add-options vect_early_break } */ +/* { dg-require-effective-target vect_early_break } */ +/* { dg-require-effective-target vect_int } */ + +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" } } */ + +char* inet_net_pton_ipv4_bits; +char inet_net_pton_ipv4_odst; +void __errno_location(); +void inet_net_pton_ipv4(); +void inet_net_pton() { inet_net_pton_ipv4(); } +void inet_net_pton_ipv4(char *dst, int size) { + while ((inet_net_pton_ipv4_bits > dst) & inet_net_pton_ipv4_odst) { + if (size-- <= 0) + goto emsgsize; + *dst++ = '\0'; + } +emsgsize: + __errno_location(); +} diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc index 30b90d99925bea74caf14833d8ab1695607d0fe9..e2587315020a35a7d4ebd3e7a9842caa36bb5d3c 100644 --- a/gcc/tree-vect-loop.cc +++ b/gcc/tree-vect-loop.cc @@ -11801,7 +11801,8 @@ move_early_exit_stmts (loop_vec_info loop_vinfo) gimple_stmt_iterator stmt_gsi = gsi_for_stmt (stmt); gsi_move_before (&stmt_gsi, &dest_gsi); - gsi_prev (&dest_gsi); + if (!gsi_end_p (dest_gsi)) + gsi_prev (&dest_gsi); } /* Update all the stmts with their new reaching VUSES. */