* [Patch] Avoid is_simple_use bug in vectorizable_live_operation @ 2015-10-26 12:54 Alan Hayward 2015-10-26 13:42 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Alan Hayward @ 2015-10-26 12:54 UTC (permalink / raw) To: gcc-patches [-- Attachment #1: Type: text/plain, Size: 874 bytes --] There is a potential bug in vectorizable_live_operation. Consider the case where the first op for stmt is valid, but the second is null. The first time through the for () loop, it will call out to vect_is_simple_use () which will set dt. The second time, because op is null, vect_is_simple_use () will not be called. However, dt is still set to a valid value, therefore the loop will continue. Note this is different from the case where the first op is null, which will cause the loop not call vect_is_simple_use () and then return false. It is possible that this was intentional, however that is not clear from the code. The fix is to simply ensure dt is initialized to a default value on each iteration. 2015-09-07 Alan Hayward alan.hayward@arm.com * tree-vect-looop.c (vectorizable_live_operation): localize variable. Cheers, Alan [-- Attachment #2: avoid_issimple.patch --] [-- Type: application/octet-stream, Size: 668 bytes --] diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c index 8c23972022d92dbfdb5742c32740233fc24684aa..6c0a919c6b056bc2588904a748fcd3ee8441cd5d 100644 --- a/gcc/tree-vect-loop.c +++ b/gcc/tree-vect-loop.c @@ -5994,7 +5994,6 @@ vectorizable_live_operation (gimple stmt, tree op; tree def; gimple def_stmt; - enum vect_def_type dt; enum tree_code code; enum gimple_rhs_class rhs_class; @@ -6059,6 +6058,8 @@ vectorizable_live_operation (gimple stmt, for (i = 0; i < op_type; i++) { + enum vect_def_type dt = vect_uninitialized_def; + if (rhs_class == GIMPLE_SINGLE_RHS) op = TREE_OPERAND (gimple_op (stmt, 1), i); else ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation 2015-10-26 12:54 [Patch] Avoid is_simple_use bug in vectorizable_live_operation Alan Hayward @ 2015-10-26 13:42 ` Richard Biener 2015-10-26 17:15 ` Alan Hayward 0 siblings, 1 reply; 6+ messages in thread From: Richard Biener @ 2015-10-26 13:42 UTC (permalink / raw) To: Alan Hayward; +Cc: gcc-patches On Mon, Oct 26, 2015 at 1:33 PM, Alan Hayward <alan.hayward@arm.com> wrote: > There is a potential bug in vectorizable_live_operation. > > Consider the case where the first op for stmt is valid, but the second is > null. > The first time through the for () loop, it will call out to > vect_is_simple_use () which will set dt. > The second time, because op is null, vect_is_simple_use () will not be > called. > However, dt is still set to a valid value, therefore the loop will > continue. > > Note this is different from the case where the first op is null, which > will cause the loop not call vect_is_simple_use () and then return false. > > It is possible that this was intentional, however that is not clear from > the code. > > The fix is to simply ensure dt is initialized to a default value on each > iteration. I think the patch is a strict improvement, thus OK. Still a NULL operand is not possible in GIMPLE so the op && check is not necessary. The way it iterates over all stmt uses is a bit scary anyway. As it is ok with all invariants it should probably simply do sth like FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) if (!vect_is_simple_use (op, ....)) and be done with that. Unvisited uses can only be constants (ok). Care to rework the funtion like that if you are here? Thanks, Richard. > > 2015-09-07 Alan Hayward alan.hayward@arm.com > * tree-vect-looop.c (vectorizable_live_operation): localize variable. > > > Cheers, > Alan > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation 2015-10-26 13:42 ` Richard Biener @ 2015-10-26 17:15 ` Alan Hayward 2015-10-27 11:37 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Alan Hayward @ 2015-10-26 17:15 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 1529 bytes --] On 26/10/2015 13:35, "Richard Biener" <richard.guenther@gmail.com> wrote: >On Mon, Oct 26, 2015 at 1:33 PM, Alan Hayward <alan.hayward@arm.com> >wrote: >> There is a potential bug in vectorizable_live_operation. >> >> Consider the case where the first op for stmt is valid, but the second >>is >> null. >> The first time through the for () loop, it will call out to >> vect_is_simple_use () which will set dt. >> The second time, because op is null, vect_is_simple_use () will not be >> called. >> However, dt is still set to a valid value, therefore the loop will >> continue. >> >> Note this is different from the case where the first op is null, which >> will cause the loop not call vect_is_simple_use () and then return >>false. >> >> It is possible that this was intentional, however that is not clear from >> the code. >> >> The fix is to simply ensure dt is initialized to a default value on each >> iteration. > >I think the patch is a strict improvement, thus OK. Still a NULL operand >is not possible in GIMPLE so the op && check is not necessary. The way >it iterates over all stmt uses is a bit scary anyway. As it is ok with >all invariants it should probably simply do sth like > > FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) > if (!vect_is_simple_use (op, ....)) > >and be done with that. Unvisited uses can only be constants (ok). > >Care to rework the funtion like that if you are here? > Ok, I’ve updated as requested. Cheers, Alan. [-- Attachment #2: avoid_issimple2.patch --] [-- Type: application/octet-stream, Size: 1480 bytes --] diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c index cc51597ce9695fb47f2fd635d22be5258d51d29b..a5125e326deab0729aa2b00c50a8aa816684066b 100644 --- a/gcc/tree-vect-loop.c +++ b/gcc/tree-vect-loop.c @@ -5901,13 +5901,12 @@ vectorizable_live_operation (gimple *stmt, stmt_vec_info stmt_info = vinfo_for_stmt (stmt); loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_info); struct loop *loop = LOOP_VINFO_LOOP (loop_vinfo); - int i; int op_type; tree op; gimple *def_stmt; - enum vect_def_type dt; enum tree_code code; enum gimple_rhs_class rhs_class; + ssa_op_iter iter; gcc_assert (STMT_VINFO_LIVE_P (stmt_info)); @@ -5967,15 +5966,11 @@ vectorizable_live_operation (gimple *stmt, /* FORNOW: support only if all uses are invariant. This means that the scalar operations can remain in place, unvectorized. The original last scalar value that they compute will be used. */ - - for (i = 0; i < op_type; i++) + FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) { - if (rhs_class == GIMPLE_SINGLE_RHS) - op = TREE_OPERAND (gimple_op (stmt, 1), i); - else - op = gimple_op (stmt, i + 1); - if (op - && !vect_is_simple_use (op, loop_vinfo, &def_stmt, &dt)) + enum vect_def_type dt = vect_uninitialized_def; + + if (!vect_is_simple_use (op, loop_vinfo, &def_stmt, &dt)) { if (dump_enabled_p ()) dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation 2015-10-26 17:15 ` Alan Hayward @ 2015-10-27 11:37 ` Richard Biener 2015-10-27 13:45 ` Alan Hayward 0 siblings, 1 reply; 6+ messages in thread From: Richard Biener @ 2015-10-27 11:37 UTC (permalink / raw) To: Alan Hayward; +Cc: gcc-patches On Mon, Oct 26, 2015 at 6:15 PM, Alan Hayward <alan.hayward@arm.com> wrote: > > > On 26/10/2015 13:35, "Richard Biener" <richard.guenther@gmail.com> wrote: > >>On Mon, Oct 26, 2015 at 1:33 PM, Alan Hayward <alan.hayward@arm.com> >>wrote: >>> There is a potential bug in vectorizable_live_operation. >>> >>> Consider the case where the first op for stmt is valid, but the second >>>is >>> null. >>> The first time through the for () loop, it will call out to >>> vect_is_simple_use () which will set dt. >>> The second time, because op is null, vect_is_simple_use () will not be >>> called. >>> However, dt is still set to a valid value, therefore the loop will >>> continue. >>> >>> Note this is different from the case where the first op is null, which >>> will cause the loop not call vect_is_simple_use () and then return >>>false. >>> >>> It is possible that this was intentional, however that is not clear from >>> the code. >>> >>> The fix is to simply ensure dt is initialized to a default value on each >>> iteration. >> >>I think the patch is a strict improvement, thus OK. Still a NULL operand >>is not possible in GIMPLE so the op && check is not necessary. The way >>it iterates over all stmt uses is a bit scary anyway. As it is ok with >>all invariants it should probably simply do sth like >> >> FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) >> if (!vect_is_simple_use (op, ....)) >> >>and be done with that. Unvisited uses can only be constants (ok). >> >>Care to rework the funtion like that if you are here? >> > > Ok, I’ve updated as requested. Ok. Please remove code = gimple_assign_rhs_code (stmt); op_type = TREE_CODE_LENGTH (code); rhs_class = get_gimple_rhs_class (code); gcc_assert (rhs_class != GIMPLE_UNARY_RHS || op_type == unary_op); gcc_assert (rhs_class != GIMPLE_BINARY_RHS || op_type == binary_op); and associated variables as I belive otherwise a --disable-checking build will fail with set-but-not-used warnings. And the asserts are quite pointless given the now sanitized loop. Thanks, Richard. > > Cheers, > Alan. > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation 2015-10-27 11:37 ` Richard Biener @ 2015-10-27 13:45 ` Alan Hayward 2015-10-27 13:52 ` Richard Biener 0 siblings, 1 reply; 6+ messages in thread From: Alan Hayward @ 2015-10-27 13:45 UTC (permalink / raw) To: Richard Biener; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 2458 bytes --] On 27/10/2015 11:36, "Richard Biener" <richard.guenther@gmail.com> wrote: >On Mon, Oct 26, 2015 at 6:15 PM, Alan Hayward <alan.hayward@arm.com> >wrote: >> >> >> On 26/10/2015 13:35, "Richard Biener" <richard.guenther@gmail.com> >>wrote: >> >>>On Mon, Oct 26, 2015 at 1:33 PM, Alan Hayward <alan.hayward@arm.com> >>>wrote: >>>> There is a potential bug in vectorizable_live_operation. >>>> >>>> Consider the case where the first op for stmt is valid, but the second >>>>is >>>> null. >>>> The first time through the for () loop, it will call out to >>>> vect_is_simple_use () which will set dt. >>>> The second time, because op is null, vect_is_simple_use () will not be >>>> called. >>>> However, dt is still set to a valid value, therefore the loop will >>>> continue. >>>> >>>> Note this is different from the case where the first op is null, which >>>> will cause the loop not call vect_is_simple_use () and then return >>>>false. >>>> >>>> It is possible that this was intentional, however that is not clear >>>>from >>>> the code. >>>> >>>> The fix is to simply ensure dt is initialized to a default value on >>>>each >>>> iteration. >>> >>>I think the patch is a strict improvement, thus OK. Still a NULL >>>operand >>>is not possible in GIMPLE so the op && check is not necessary. The way >>>it iterates over all stmt uses is a bit scary anyway. As it is ok with >>>all invariants it should probably simply do sth like >>> >>> FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) >>> if (!vect_is_simple_use (op, ....)) >>> >>>and be done with that. Unvisited uses can only be constants (ok). >>> >>>Care to rework the funtion like that if you are here? >>> >> >> Ok, I’ve updated as requested. > >Ok. Please remove > > code = gimple_assign_rhs_code (stmt); > op_type = TREE_CODE_LENGTH (code); > rhs_class = get_gimple_rhs_class (code); > gcc_assert (rhs_class != GIMPLE_UNARY_RHS || op_type == unary_op); > gcc_assert (rhs_class != GIMPLE_BINARY_RHS || op_type == binary_op); > >and associated variables as I belive otherwise a --disable-checking build >will fail with set-but-not-used warnings. And the asserts are quite >pointless >given the now sanitized loop. > I did consider removing those asserts, but didn’t want to remove any important checks. Didn’t think about the disable-checking build. Anyway, new patch attached. Cheers, Alan. [-- Attachment #2: avoid_issimple3.patch --] [-- Type: application/octet-stream, Size: 1810 bytes --] diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c index cc51597ce9695fb47f2fd635d22be5258d51d29b..32d0bb580c00ecc7f1d34c149785fd2bcec016ca 100644 --- a/gcc/tree-vect-loop.c +++ b/gcc/tree-vect-loop.c @@ -5901,13 +5901,9 @@ vectorizable_live_operation (gimple *stmt, stmt_vec_info stmt_info = vinfo_for_stmt (stmt); loop_vec_info loop_vinfo = STMT_VINFO_LOOP_VINFO (stmt_info); struct loop *loop = LOOP_VINFO_LOOP (loop_vinfo); - int i; - int op_type; tree op; gimple *def_stmt; - enum vect_def_type dt; - enum tree_code code; - enum gimple_rhs_class rhs_class; + ssa_op_iter iter; gcc_assert (STMT_VINFO_LIVE_P (stmt_info)); @@ -5958,24 +5954,14 @@ vectorizable_live_operation (gimple *stmt, if (nested_in_vect_loop_p (loop, stmt)) return false; - code = gimple_assign_rhs_code (stmt); - op_type = TREE_CODE_LENGTH (code); - rhs_class = get_gimple_rhs_class (code); - gcc_assert (rhs_class != GIMPLE_UNARY_RHS || op_type == unary_op); - gcc_assert (rhs_class != GIMPLE_BINARY_RHS || op_type == binary_op); - /* FORNOW: support only if all uses are invariant. This means that the scalar operations can remain in place, unvectorized. The original last scalar value that they compute will be used. */ - - for (i = 0; i < op_type; i++) + FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) { - if (rhs_class == GIMPLE_SINGLE_RHS) - op = TREE_OPERAND (gimple_op (stmt, 1), i); - else - op = gimple_op (stmt, i + 1); - if (op - && !vect_is_simple_use (op, loop_vinfo, &def_stmt, &dt)) + enum vect_def_type dt = vect_uninitialized_def; + + if (!vect_is_simple_use (op, loop_vinfo, &def_stmt, &dt)) { if (dump_enabled_p ()) dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation 2015-10-27 13:45 ` Alan Hayward @ 2015-10-27 13:52 ` Richard Biener 0 siblings, 0 replies; 6+ messages in thread From: Richard Biener @ 2015-10-27 13:52 UTC (permalink / raw) To: Alan Hayward; +Cc: gcc-patches On Tue, Oct 27, 2015 at 2:35 PM, Alan Hayward <alan.hayward@arm.com> wrote: > > > On 27/10/2015 11:36, "Richard Biener" <richard.guenther@gmail.com> wrote: > >>On Mon, Oct 26, 2015 at 6:15 PM, Alan Hayward <alan.hayward@arm.com> >>wrote: >>> >>> >>> On 26/10/2015 13:35, "Richard Biener" <richard.guenther@gmail.com> >>>wrote: >>> >>>>On Mon, Oct 26, 2015 at 1:33 PM, Alan Hayward <alan.hayward@arm.com> >>>>wrote: >>>>> There is a potential bug in vectorizable_live_operation. >>>>> >>>>> Consider the case where the first op for stmt is valid, but the second >>>>>is >>>>> null. >>>>> The first time through the for () loop, it will call out to >>>>> vect_is_simple_use () which will set dt. >>>>> The second time, because op is null, vect_is_simple_use () will not be >>>>> called. >>>>> However, dt is still set to a valid value, therefore the loop will >>>>> continue. >>>>> >>>>> Note this is different from the case where the first op is null, which >>>>> will cause the loop not call vect_is_simple_use () and then return >>>>>false. >>>>> >>>>> It is possible that this was intentional, however that is not clear >>>>>from >>>>> the code. >>>>> >>>>> The fix is to simply ensure dt is initialized to a default value on >>>>>each >>>>> iteration. >>>> >>>>I think the patch is a strict improvement, thus OK. Still a NULL >>>>operand >>>>is not possible in GIMPLE so the op && check is not necessary. The way >>>>it iterates over all stmt uses is a bit scary anyway. As it is ok with >>>>all invariants it should probably simply do sth like >>>> >>>> FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) >>>> if (!vect_is_simple_use (op, ....)) >>>> >>>>and be done with that. Unvisited uses can only be constants (ok). >>>> >>>>Care to rework the funtion like that if you are here? >>>> >>> >>> Ok, I’ve updated as requested. >> >>Ok. Please remove >> >> code = gimple_assign_rhs_code (stmt); >> op_type = TREE_CODE_LENGTH (code); >> rhs_class = get_gimple_rhs_class (code); >> gcc_assert (rhs_class != GIMPLE_UNARY_RHS || op_type == unary_op); >> gcc_assert (rhs_class != GIMPLE_BINARY_RHS || op_type == binary_op); >> >>and associated variables as I belive otherwise a --disable-checking build >>will fail with set-but-not-used warnings. And the asserts are quite >>pointless >>given the now sanitized loop. >> > > I did consider removing those asserts, but didn’t want to remove any > important checks. Didn’t think about the disable-checking build. > Anyway, new patch attached. Ok. Richard. > > Cheers, > Alan. > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-10-27 13:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-26 12:54 [Patch] Avoid is_simple_use bug in vectorizable_live_operation Alan Hayward 2015-10-26 13:42 ` Richard Biener 2015-10-26 17:15 ` Alan Hayward 2015-10-27 11:37 ` Richard Biener 2015-10-27 13:45 ` Alan Hayward 2015-10-27 13:52 ` 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).