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


2012/2/9 Richard Guenther <richard.guenther@gmail.com>:
> 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.

Hmm, I see in libstdc++'s file include/bits/boost_concept_check.h some
use of '*__i++ = *__i;' and '*__i-- = *__i;', which seems to cause
part of this failure.

This might lead here to this failure.  I am not sure, if such
constructs having fixed behavior for C++, but it looks to me like
undefined behavior.

A C-testcase for the issue would be:

int *foo (int *p)
{
  *p++ = *p;
  return p;
}

which produces with patch now:

foo (int * p)
{
  int * D.1363;
  int D.1364;
  int * D.1365;

  D.1363 = p;
  p = D.1363 + 4;
  D.1364 = *p;
  *D.1363 = D.1364;
  D.1365 = p;
  return D.1365;
}

but in old variant we were producing:

foo (int * p)
{
  int D.1363;
  int * D.1364;

  D.1363 = *p;
  *p = D.1363;
  p = p + 4;
  D.1364 = p;
  return D.1364;
}

So, maybe the real solution for this issue might be to swap for
assignment gimplification the order for lhs/rhs gimplification
instead.


Regards,
Kai


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