Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation

[Richard Biener <richard dot guenther at gmail dot com>]

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