* [shrink-wrap] should not sink instructions which may cause trap ? @ 2014-09-25 22:49 Jiong Wang 2014-09-26 8:36 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Jiong Wang @ 2014-09-25 22:49 UTC (permalink / raw) To: Jeff Law, christophe.lyon; +Cc: gcc-patches 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? I will try to send a fix tomorrow. thanks. -- Jiong > > -- Jiong > > >> >> Here is what I have in my logs: >> >> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../xg++ >> >> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../ >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C >> -fno-diagnostics-show-caret -fdiagnostics-color=never -nostdinc++ >> >> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include/arm-none-linux-gnueabihf >> >> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include >> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++ >> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/backward >> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/util >> -fmessage-length=0 -std=gnu++98 -O -fnon-call-exceptions -S -o >> pr49847.s (timeout = 800) >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C: In >> function 'int f(float)': >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1: >> error: missing REG_EH_REGION note at the end of bb 2 >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1: >> internal compiler error: verify_flow_info failed >> 0x82f8ba verify_flow_info() >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfghooks.c:260 >> >> 0x840cd3 commit_edge_insertions() >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgrtl.c:2068 >> 0x9bf243 thread_prologue_and_epilogue_insns >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:5852 >> 0x9bfa52 rest_of_handle_thread_prologue_and_epilogue >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6245 >> 0x9bfa52 execute >> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6283 >> >> As per >> >> http://cbuild.validation.linaro.org/build/cross-validation/gcc/trunk/215563/report-build-info.html >> I've noticed this on targets: >> arm-none-linux-gnueabihf >> armeb-none-linux-gnueabihf >> aarch64-none-elf >> aarch64_be-none-elf >> aarch64-none-linux-gnu >> but NOT on >> arm-none-eabi >> arm-none-linux-gnueabi >> >> Christophe. >> > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [shrink-wrap] should not sink instructions which may cause trap ? 2014-09-25 22:49 [shrink-wrap] should not sink instructions which may cause trap ? Jiong Wang @ 2014-09-26 8:36 ` Richard Biener 2014-09-26 14:45 ` Jiong Wang 0 siblings, 1 reply; 8+ messages in thread From: Richard Biener @ 2014-09-26 8:36 UTC (permalink / raw) To: Jiong Wang; +Cc: Jeff Law, christophe.lyon, gcc-patches 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. > I will try to send a fix tomorrow. > > thanks. > > -- Jiong > > >> >> -- Jiong >> >> >>> >>> Here is what I have in my logs: >>> >>> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../xg++ >>> >>> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../ >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C >>> -fno-diagnostics-show-caret -fdiagnostics-color=never -nostdinc++ >>> >>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include/arm-none-linux-gnueabihf >>> >>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include >>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++ >>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/backward >>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/util >>> -fmessage-length=0 -std=gnu++98 -O -fnon-call-exceptions -S -o >>> pr49847.s (timeout = 800) >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C: In >>> function 'int f(float)': >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1: >>> error: missing REG_EH_REGION note at the end of bb 2 >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1: >>> internal compiler error: verify_flow_info failed >>> 0x82f8ba verify_flow_info() >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfghooks.c:260 >>> >>> 0x840cd3 commit_edge_insertions() >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgrtl.c:2068 >>> 0x9bf243 thread_prologue_and_epilogue_insns >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:5852 >>> 0x9bfa52 rest_of_handle_thread_prologue_and_epilogue >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6245 >>> 0x9bfa52 execute >>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6283 >>> >>> As per >>> >>> http://cbuild.validation.linaro.org/build/cross-validation/gcc/trunk/215563/report-build-info.html >>> I've noticed this on targets: >>> arm-none-linux-gnueabihf >>> armeb-none-linux-gnueabihf >>> aarch64-none-elf >>> aarch64_be-none-elf >>> aarch64-none-linux-gnu >>> but NOT on >>> arm-none-eabi >>> arm-none-linux-gnueabi >>> >>> Christophe. >>> >> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [shrink-wrap] should not sink instructions which may cause trap ? 2014-09-26 8:36 ` Richard Biener @ 2014-09-26 14:45 ` Jiong Wang 2014-09-26 14:50 ` Jiong Wang 0 siblings, 1 reply; 8+ messages in thread From: Jiong Wang @ 2014-09-26 14:45 UTC (permalink / raw) To: Richard Biener, Jeff Law; +Cc: christophe.lyon, gcc-patches [-- Attachment #1: Type: text/plain, Size: 5295 bytes --] 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? -- Jiong > > Richard. > >> I will try to send a fix tomorrow. >> >> thanks. >> >> -- Jiong >> >> >>> -- Jiong >>> >>> >>>> Here is what I have in my logs: >>>> >>>> /aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../xg++ >>>> >>>> -B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/gcc/testsuite/g++/../../ >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C >>>> -fno-diagnostics-show-caret -fdiagnostics-color=never -nostdinc++ >>>> >>>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include/arm-none-linux-gnueabihf >>>> >>>> -I/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabihf/gcc3/arm-none-linux-gnueabihf/libstdc++-v3/include >>>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/libsupc++ >>>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/include/backward >>>> -I/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libstdc++-v3/testsuite/util >>>> -fmessage-length=0 -std=gnu++98 -O -fnon-call-exceptions -S -o >>>> pr49847.s (timeout = 800) >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C: In >>>> function 'int f(float)': >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1: >>>> error: missing REG_EH_REGION note at the end of bb 2 >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/testsuite/g++.dg/pr49847.C:7:1: >>>> internal compiler error: verify_flow_info failed >>>> 0x82f8ba verify_flow_info() >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfghooks.c:260 >>>> >>>> 0x840cd3 commit_edge_insertions() >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/cfgrtl.c:2068 >>>> 0x9bf243 thread_prologue_and_epilogue_insns >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:5852 >>>> 0x9bfa52 rest_of_handle_thread_prologue_and_epilogue >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6245 >>>> 0x9bfa52 execute >>>> /aci-gcc-fsf/sources/gcc-fsf/gccsrc/gcc/function.c:6283 >>>> >>>> As per >>>> >>>> http://cbuild.validation.linaro.org/build/cross-validation/gcc/trunk/215563/report-build-info.html >>>> I've noticed this on targets: >>>> arm-none-linux-gnueabihf >>>> armeb-none-linux-gnueabihf >>>> aarch64-none-elf >>>> aarch64_be-none-elf >>>> aarch64-none-linux-gnu >>>> but NOT on >>>> arm-none-eabi >>>> arm-none-linux-gnueabi >>>> >>>> Christophe. >>>> >>> [-- 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) { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [shrink-wrap] should not sink instructions which may cause trap ? 2014-09-26 14:45 ` Jiong Wang @ 2014-09-26 14:50 ` Jiong Wang 2014-09-26 16:13 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Jiong Wang @ 2014-09-26 14:50 UTC (permalink / raw) To: Richard Biener, Jeff Law; +Cc: christophe.lyon, gcc-patches [-- 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) { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [shrink-wrap] should not sink instructions which may cause trap ? 2014-09-26 14:50 ` Jiong Wang @ 2014-09-26 16:13 ` Jeff Law 2014-09-29 18:07 ` Jiong Wang 0 siblings, 1 reply; 8+ messages in thread From: Jeff Law @ 2014-09-26 16:13 UTC (permalink / raw) To: Jiong Wang, Richard Biener; +Cc: christophe.lyon, gcc-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [shrink-wrap] should not sink instructions which may cause trap ? 2014-09-26 16:13 ` Jeff Law @ 2014-09-29 18:07 ` Jiong Wang 2014-09-30 4:26 ` Jeff Law 0 siblings, 1 reply; 8+ messages in thread From: Jiong Wang @ 2014-09-29 18:07 UTC (permalink / raw) To: Jeff Law, Richard Biener; +Cc: christophe.lyon, gcc-patches [-- Attachment #1: Type: text/plain, Size: 2129 bytes --] 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. [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 1.patch --] [-- Type: text/x-patch; name=1.patch, Size: 594 bytes --] 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) { ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [shrink-wrap] should not sink instructions which may cause trap ? 2014-09-29 18:07 ` Jiong Wang @ 2014-09-30 4:26 ` Jeff Law 2014-09-30 8:51 ` [COMMITTED][shrink-wrap] " Jiong Wang 0 siblings, 1 reply; 8+ messages in thread From: Jeff Law @ 2014-09-30 4:26 UTC (permalink / raw) To: Jiong Wang, Richard Biener; +Cc: christophe.lyon, gcc-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [COMMITTED][shrink-wrap] should not sink instructions which may cause trap ? 2014-09-30 4:26 ` Jeff Law @ 2014-09-30 8:51 ` Jiong Wang 0 siblings, 0 replies; 8+ messages in thread From: Jiong Wang @ 2014-09-30 8:51 UTC (permalink / raw) To: Jeff Law, Richard Biener; +Cc: christophe.lyon, gcc-patches 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-09-30 8:51 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-09-25 22:49 [shrink-wrap] should not sink instructions which may cause trap ? Jiong Wang 2014-09-26 8:36 ` Richard Biener 2014-09-26 14:45 ` Jiong Wang 2014-09-26 14:50 ` Jiong Wang 2014-09-26 16:13 ` Jeff Law 2014-09-29 18:07 ` Jiong Wang 2014-09-30 4:26 ` Jeff Law 2014-09-30 8:51 ` [COMMITTED][shrink-wrap] " Jiong Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).