Re: [PATCH][TER] PR 48863: Don't replace expressions across local register variable definitions

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

On Thu, Dec 1, 2016 at 12:14 PM, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>
> On 24/11/16 15:12, Richard Biener wrote:
>>
>> On Thu, Nov 24, 2016 at 2:57 PM, Kyrill Tkachov
>> <kyrylo.tkachov@foss.arm.com> wrote:
>>>
>>> Hi all,
>>>
>>> In this bug we have TER during out-of-ssa misbehaving. A minimal example
>>> is:
>>> void foo(unsigned a, float b)
>>> {
>>>    unsigned c = (unsigned) b;      // 1
>>>    register unsigned r0 __asm__("r0") = a; // 2
>>>    register unsigned r1 __asm__("r1") = c; // 3
>>>      __asm__ volatile( "str %[r1], [%[r0]]\n"
>>>                        :
>>>                        : [r0] "r" (r0),
>>>                          [r1] "r" (r1));
>>> }
>>>
>>> Statement 1 can produce a libcall to convert float b into an int and TER
>>> moves it
>>> into statement 3. But the libcall clobbers r0, which we want set to 'a'
>>> in
>>> statement 2.
>>> So r0 gets clobbered by the argument to the conversion libcall.
>>>
>>> TER already has code to avoid substituting across function calls and
>>> ideally
>>> we'd teach it
>>> to not substitute expressions that can perform a libcall across register
>>> variable definitions
>>> where the register can be clobbered in a libcall, but that information is
>>> not easy to get hold
>>> off in a general way at this level.
>>>
>>> So this patch disallows replacement across any local register variable
>>> definition. It does this
>>> by keeping track of local register definitions encountered in a similar
>>> way
>>> to which calls are
>>> counted for TER purposes.
>>>
>>> I hope this is not too big a hammer as local register variables are not
>>> very
>>> widely used and we
>>> only disable replacement across them and it allows us to fix the
>>> wrong-code
>>> bug on some
>>> inline-asm usage scenarios where gcc currently miscompiles code following
>>> its documented
>>> advice [1]
>>>
>>> Bootstrapped and tested on arm-none-linux-gnueabihf,
>>> aarch64-none-linux-gnu,
>>> x86_64.
>>>
>>> Is this approach acceptable?
>>
>> Ok.
>
>
> Thanks.
> This has been in trunk for a week without any problems.
> Is it ok to backport to the branches?
> I have bootstrapped and tested arm-none-linux-gnueabihf on them.

I think so.

Richard.

> Kyrill
>
>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> Kyrill
>>>
>>> [1]
>>>
>>> https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables
>>>
>>> 2016-11-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      PR target/48863
>>>      PR inline-asm/70184
>>>      * tree-ssa-ter.c (temp_expr_table): Add reg_vars_cnt field.
>>>      (new_temp_expr_table): Initialise reg_vars_cnt.
>>>      (free_temp_expr_table): Release reg_vars_cnt.
>>>      (process_replaceable): Add reg_vars_cnt argument, set reg_vars_cnt
>>>      field of TAB.
>>>      (find_replaceable_in_bb): Use the above to record register variable
>>>      write occurrences and cancel replacement across them.
>>>
>>> 2016-11-24  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>>
>>>      PR target/48863
>>>      PR inline-asm/70184
>>>      * gcc.target/arm/pr48863.c: New test.
>
>