Re: [PR72835] Incorrect arithmetic optimization involving bitfield arguments

[kugan <kugan.vivekanandarajah@linaro.org>]

On 10/08/16 08:51, kugan wrote:
> Hi Jakub,
>
> On 10/08/16 07:55, Jakub Jelinek wrote:
>> On Wed, Aug 10, 2016 at 07:51:08AM +1000, kugan wrote:
>>> On 10/08/16 07:46, Jakub Jelinek wrote:
>>>> On Wed, Aug 10, 2016 at 07:42:25AM +1000, kugan wrote:
>>>>> There was no new regression while testing. I also moved the testcase from
>>>>> gcc.dg/torture/pr72835.c to gcc.dg/tree-ssa/pr72835.c. Is this OK for trunk?
>>>>
>>>> This looks strange.  The tree-ssa-reassoc.c code has been trying to never
>>>> reuse SSA_NAMEs if they would hold a different value.
>>>> So there should be no resetting of flow sensitive info needed.
>>>
>>> We are not reusing but, if you see the example change in reassoc:
>>>
>>> -  _5 = -_4;
>>> -  _6 = _2 * _5;
>>> +  _5 = _4;
>>> +  _6 = _5 * _2;
>>>
>>> _5 and _6 will now have different value ranges because they compute
>>> different values. Therefore I think we should reset (?).
>>
>> No.  We should not have reused _5 and _6 for the different values.
>> It is not harmful just for the value ranges, but also for debug info.
>
> I see it now. The problem is we are just looking at (-1) being in the
> ops list for passing changed to rewrite_expr_tree in the case of
> multiplication by negate.  If we have combined (-1), as in the testcase,
> we will not have the (-1) and will pass changed=false to rewrite_expr_tree.
>
> We should set changed based on what happens in try_special_add_to_ops.
> Attached patch does this. Bootstrap and regression testing are ongoing.
> Is this OK for trunk if there is no regression.
>

This patch unfortunately does not handle all the cases. I am revising 
it. Sorry for the noise.

Thanks,
Kugan