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

[Jiong Wang <jiong.wang@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 3125 bytes --]

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.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix-except.patch --]
[-- Type: text/x-patch; name=fix-except.patch, Size: 452 bytes --]

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)
 	{