On Tue, Oct 27, 2015 at 2:35 PM, Alan Hayward <alan.hayw...@arm.com> wrote: > > > On 27/10/2015 11:36, "Richard Biener" <richard.guent...@gmail.com> wrote: > >>On Mon, Oct 26, 2015 at 6:15 PM, Alan Hayward <alan.hayw...@arm.com> >>wrote: >>> >>> >>> On 26/10/2015 13:35, "Richard Biener" <richard.guent...@gmail.com> >>>wrote: >>> >>>>On Mon, Oct 26, 2015 at 1:33 PM, Alan Hayward <alan.hayw...@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. >
