Re: Ping: Re: [patch middle-end]: Fix PR/48814 - [4.4/4.5/4.6/4.7 Regression] Incorrect scalar increment result

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

On Thu, Feb 9, 2012 at 2:48 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
>> On Thu, Feb 9, 2012 at 11:53 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Feb 9, 2012 at 11:29 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Feb 8, 2012 at 10:25 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>>> 2012/1/11 Richard Guenther <richard.guenther@gmail.com>:
>>>>>>
>>>>>> count despite being declared volatile and only loaded once in the source
>>>>>> is loaded twice in gimple. ?If it were a HW register which destroys the
>>>>>> device after the 2nd load without an intervening store you'd wrecked
>>>>>> the device ;)
>>>>>>
>>>>>> Richard.
>>>>>
>>>>> Thanks for explaination. ?I tried to flip order for lhs/rhs in
>>>>> gimplify_modify_expr & co. ?Issue here is that for some cases we are
>>>>> relying here on lhs for gimplifying rhs (is_gimple_reg_rhs_or_call vs
>>>>> is_gimple_mem_rhs_or_call) and this doesn't work for cases in C++
>>>>> like:
>>>>>
>>>>> typedef const unsigned char _Jv_Utf8Const;
>>>>> typedef __SIZE_TYPE__ uaddr;
>>>>>
>>>>> void maybe_adjust_signature (_Jv_Utf8Const *&s, uaddr &special)
>>>>> {
>>>>> ?union {
>>>>> ? ?_Jv_Utf8Const *signature;
>>>>> ? ?uaddr signature_bits;
>>>>> ?};
>>>>> ?signature = s;
>>>>> ?special = signature_bits & 1;
>>>>> ?signature_bits -= special;
>>>>> ?s = signature;
>>>>> }
>>>>>
>>>>> So I modified gimplify_self_mod_expr for post-inc/dec so that we use
>>>>> following sequence
>>>>> and add it to pre_p for it:
>>>>>
>>>>> tmp = lhs;
>>>>> lvalue = tmp (+/-) rhs
>>>>> *expr_p = tmp;
>>>>
>>>> As I explained this is the wrong place to fix the PR. ?The issue is not
>>>> about self-modifying expressions but about evaluating call argument
>>>> side-effects before side-effects of the lhs.
>>>
>>> I am testing the attached instead.
>>
>> Doesn't work. ?Btw, Kai, your patch surely breaks things if you put
>> the lvalue update into the pre queue.
>>
>> Consider a simple
>>
>> ?a[i++] = i;
>>
>> you gimplify that to
>>
>> ?i.0 = i;
>> ?D.1709 = i.0;
>> ?i = D.1709 + 1;
>> ?a[D.1709] = i;
>>
>> which is wrong.
>>
>> Seems we are lacking some basic pre-/post-modify testcases ...
>>
>> Richard.
>
> Why, this should be wrong? ?In fact C specification just says that the
> post-inc has to happen at least before next sequence-point. ?It
> doesn't say that the increment has to happen after evaluation of rhs.
>
> The produced gimple for the following C-code
>
> int arr[128];
>
> void foo (int i)
> {
> ?arr[i++] = i;
> }
>
> is:
>
> foo (int i)
> {
> ?int D.1364;
>
> ?D.1364 = i;
> ?i = D.1364 + 1;
> ?arr[D.1364] = i;
> }
>
> which looks to me from description of the C-specification correct.

Hm, indeed.  I'll test the following shorter patch and add the struct-return
volatile testcase.

Richard.

Attachment: fix-pr48814
Description: Binary data