Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation

[Alan Hayward <alan dot hayward at arm dot com>]

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: avoid_issimple3.patch
Description: Binary data