Re: [Patch] Avoid is_simple_use bug in vectorizable_live_operation

[Richard Biener <richard.guenther@gmail.com>]

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