This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Thu, Feb 9, 2012 at 3:41 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> 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.

Works apart from

Running target unix/
FAIL: ext/pb_ds/regression/trie_map_rand.cc execution test
FAIL: ext/pb_ds/regression/trie_set_rand.cc execution test

Maybe invalid testcases.  Who knows ... same fails happen with your patch.

Richard.

> Richard.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]