diff mbox

[shrink-wrap] should not sink instructions which may cause trap ?

Message ID 54299FC2.2080805@arm.com
State New
Headers show

Commit Message

Jiong Wang Sept. 29, 2014, 6:06 p.m. UTC 
On 26/09/14 17:12, Jeff Law wrote:
> On 09/26/14 08:50, Jiong Wang wrote:
>
>>>>>      if (may_trap_p (x))
>>>>>        don't sink this instruction.
>>>>>
>>>>>       any comments?
>>>> Should be checking if x may throw internally instead.
>>> Richard, thanks for the suggestion, have used insn_could_throw_p to do
>>> the check,
>>> which will only do the check when flag_exception and
>>> flag_non_call_exception be true,
>>> so those instruction could still be sink for normal c/c++ program.
>>>
>>> Jeff,
>>>
>>>      below is the fix for pr49847.C regression on aarch64. I re-run
>>> full test on
>>>      aarch64-none-elf bare metal, no regression.
>>>
>>>      bootstrap ok on x86, no regression on check-gcc/g++.
>>>
>>>      ok for trunk?
>> (re-sent with changelog entry)
>>
>> gcc/
>>
>> 2014-09-26  Jiong Wang<jiong.wang@arm.com>
>>
>>           * shrink-wrap.c (move_insn_for_shrink_wrap): Check
>> "insn_could_throw_p" before
>>           sinking insn.
> I think can_throw_internal, per Richi's recommendation is better.
>
> Note that can_throw_internal keys off the existence of the EH landing
> pads for the particular insn.
>
> If flag_exceptions is false (for example), then would not expect those
> landing pads to exist and the insn would not be considered as
> potentially throwing.
>
> Can you test with can_throw_internal to verify it's behaviour and resubmit

thanks for pointing this out, patch updated.

re-tested, pass x86-64 bootstrap and no regression on check-gcc/g++.
pass aarch64-none-elf cross check also.

ok for trunk?

BTW, another bug exposed by linux x86-64 kernel build, and it's at

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63404

the problem is caused by we missed clobber/use check. I will send
a seperate patch for review. really sorry for causing the trouble,
the insn move in generic code is actually not that generic, related
with some backend features...

2014-09-26  Jiong Wang  <jiong.wang@arm.com>

         * shrink-wrap.c (move_insn_for_shrink_wrap): Check "can_throw_internal" before
         sinking insn.

Comments

Jeff Law Sept. 30, 2014, 4:26 a.m. UTC | #1 
On 09/29/14 12:06, Jiong Wang wrote:
>
> thanks for pointing this out, patch updated.
>
> re-tested, pass x86-64 bootstrap and no regression on check-gcc/g++.
> pass aarch64-none-elf cross check also.
>
> ok for trunk?
Yes this is fine.

>
> BTW, another bug exposed by linux x86-64 kernel build, and it's at
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63404
>
> the problem is caused by we missed clobber/use check. I will send
> a seperate patch for review. really sorry for causing the trouble,
> the insn move in generic code is actually not that generic, related
> with some backend features...
Noted.  These things happen, it's one of the things that makes working 
with RTL tough and one of the reasons we made a major focus away from 
RTL as the primary IL for optimization work.  But for things like 
shrink-wrapping, RTL is the right place to be.

jeff
Jiong Wang Sept. 30, 2014, 8:51 a.m. UTC | #2 
On 30/09/14 05:26, Jeff Law wrote:
> On 09/29/14 12:06, Jiong Wang wrote:
>> thanks for pointing this out, patch updated.
>>
>> re-tested, pass x86-64 bootstrap and no regression on check-gcc/g++.
>> pass aarch64-none-elf cross check also.
>>
>> ok for trunk?
> Yes this is fine.

committed as 215709.

>
>> BTW, another bug exposed by linux x86-64 kernel build, and it's at
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63404
>>
>> the problem is caused by we missed clobber/use check. I will send
>> a seperate patch for review. really sorry for causing the trouble,
>> the insn move in generic code is actually not that generic, related
>> with some backend features...
> Noted.  These things happen, it's one of the things that makes working
> with RTL tough and one of the reasons we made a major focus away from
> RTL as the primary IL for optimization work.  But for things like
> shrink-wrapping, RTL is the right place to be.

thanks for the explanation. Now  I fell I get a deeper understanding of why it's
called RTL, the "Register Transfer Language" :)

>
> jeff
>
>
diff mbox

Patch

commit bff6072abd52fecde5916d1967a7833f581c1e98
Author: Jiong Wang <jiong.wang@arm.com>
Date:   Mon Sep 29 13:32:02 2014 +0100

    1

diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c
index bd4813c..b1ff8a2 100644
--- a/gcc/shrink-wrap.c
+++ b/gcc/shrink-wrap.c
@@ -189,6 +189,9 @@  move_insn_for_shrink_wrap (basic_block bb, rtx_insn *insn,
       unsigned int nonconstobj_num = 0;
       rtx src_inner = NULL_RTX;
 
+      if (can_throw_internal (insn))
+	return false;
+
       subrtx_var_iterator::array_type array;
       FOR_EACH_SUBRTX_VAR (iter, array, src, ALL)
 	{