diff mbox

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

Message ID 54257D2A.1070103@arm.com
State New
Headers show

Commit Message

Jiong Wang Sept. 26, 2014, 2:50 p.m. UTC 
On 26/09/14 15:45, Jiong Wang wrote:
> On 26/09/14 09:36, Richard Biener wrote:
>> On Fri, Sep 26, 2014 at 12:49 AM, Jiong Wang
>> <wong.kwongyuan.tools@gmail.com> wrote:
>>> 2014-09-25 14:07 GMT+01:00 Jiong Wang <jiong.wang@arm.com>:
>>>> On 25/09/14 12:25, Christophe Lyon wrote:
>>>>> I have observed regressions in the g++ testsuite: pr49847 now FAILs
>>>>> after this patch.
>>>> no.
>>>>
>>>> even without my patch, the regression still happen.
>>>>
>>>> or you could specify -fno-shrink-wrap, gcc still crash.
>>>>
>>>> so, this regression should caused by other commits which haven't exposed on
>>>> x86 regression test.
>>> sorry, confirmed, there is regression.
>>>
>>> my code was git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@215590.
>>> there also be gcc crash on aarch64, with the following info,
>>>     pr49847.C:5:21: internal compiler error: Segmentation fault
>>>        try { return g >= 0; }
>>>                        ^
>>>     0xdc249e crash_signal
>>>         ../../gcc/gcc/toplev.c:340
>>>     0xdbfeff default_get_reg_raw_mode(int)
>>>
>>> so I was thinking it's caused by other commits instead of this, and after I sync
>>> code to git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@215599 I could
>>> reproduce this bug.
>>>
>>>     error: missing REG_EH_REGION note at the end of bb 2
>>>
>>> the reson is:
>>>     * before this patch, we only sink simple "set reg, reg" instruction which
>>>       the corresponding instruction will not produce exception, thus no
>>> REG_EH_REGION attached.
>>>     * after this patch, we will sink instruction like the following for
>>> aarch64 or arm or other RISC.
>>>
>>>       (insn 7 3 30 2 (set (reg:CCFPE 66 cc)
>>>           (compare:CCFPE (reg:SF 32 v0 [ g ])
>>>               (const_double:SF 0.0 [0x0.0p+0]))) pr49847.C:5 330 {*cmpesf}
>>>        (expr_list:REG_DEAD (reg:SF 32 v0 [ g ])
>>>           (expr_list:REG_EH_REGION (const_int 1 [0x1])
>>>               (nil))))
>>>
>>>     "compare" is actually a operator which may cause trap and we need to prevent
>>>     any instruction which may causing trap be sink, because that may
>>> break exception handling logic
>>>
>>>     so something like the following should be added to the iterator check
>>>
>>>     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.

Comments

Jeff Law Sept. 26, 2014, 4:12 p.m. UTC | #1 
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


jeff
diff mbox

Patch

diff --git a/gcc/shrink-wrap.c b/gcc/shrink-wrap.c
index bd4813c..2e2f0a6 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 (insn_could_throw_p (insn))
+	return false;
+
       subrtx_var_iterator::array_type array;
       FOR_EACH_SUBRTX_VAR (iter, array, src, ALL)
 	{