| Message ID | 530B0CB0.8090201@st.com |
|---|---|
| State | New |
| Headers | show |
Please also check the two test cases in patch https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg72712.html Thanks! -Zhenqiang On 24 February 2014 17:11, Christian Bruel <christian.bruel@st.com> wrote: > This patch improves the one sent previously, > (http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01159.html), to fix a few > more failures in the testsuite that could arise with shrink-wrap and > -fexceptions. > > To recall, the problem that it fixes is that with -mapcs-frame : > > - the epilogue pops as > > sub sp, fp, #12 @ does not set FRAME_RELATED_P > ldmia sp, {fp, sp, lr} @ XXX assert def_cfa->reg is FP instead > of SP > > - with vrp this is worse, we have > > fldmfdd ip!, {d8} @ FRAME_RELATED_P > sub sp, fp, #20 ... > ldmfd sp, {r3, r4, fp, sp, pc} @ XXX assert def_cfa->reg is IP > instead of SP, > > Fixed by inserting a REG_CFA_DEF_CFA note, fixing the arm_unwind_emit > machinery and setting the FRAME_RELATED_P . The comment says : > > /* The INSN is generated in epilogue. It is set as RTX_FRAME_RELATED_P > to get correct dwarf information for shrink-wrap. We should not > emit unwind information for it because these are used either for > pretend arguments or notes to adjust sp and restore registers from > stack. */ > > the testsuite score improves without regression (improvements from -g > and -fexeptions tests) > > === gcc Summary for arm-sim//-mapcs-frame === > > # of expected passes 77545 > # of unexpected failures 31 > # of unexpected successes 2 > # of expected failures 172 > # of unsupported tests 1336 > > === g++ Summary for arm-sim//-mapcs-frame === > > # of expected passes 50116 > # of unexpected failures 9 > # of unexpected successes 3 > # of expected failures 280 > # of unsupported tests 1229 > > instead of > > === gcc Summary for arm-sim//-mapcs-frame === > > # of expected passes 77106 > # of unexpected failures 500 > # of unexpected successes 2 > # of expected failures 172 > # of unresolved testcases 111 > # of unsupported tests 1336 > > === g++ Summary for arm-sim//-mapcs-frame === > > # of expected passes 50021 > # of unexpected failures 136 > # of unexpected successes 3 > # of expected failures 280 > # of unsupported tests 1229 > > Comments ? OK for trunk ? > > Many thanks > >
On 02/24/2014 11:11 AM, Zhenqiang Chen wrote: > Please also check the two test cases in patch > https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg72712.html Just cheched, they both now pass. Cheers, > > Thanks! > -Zhenqiang > > On 24 February 2014 17:11, Christian Bruel <christian.bruel@st.com> wrote: >> This patch improves the one sent previously, >> (http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01159.html), to fix a few >> more failures in the testsuite that could arise with shrink-wrap and >> -fexceptions. >> >> To recall, the problem that it fixes is that with -mapcs-frame : >> >> - the epilogue pops as >> >> sub sp, fp, #12 @ does not set FRAME_RELATED_P >> ldmia sp, {fp, sp, lr} @ XXX assert def_cfa->reg is FP instead >> of SP >> >> - with vrp this is worse, we have >> >> fldmfdd ip!, {d8} @ FRAME_RELATED_P >> sub sp, fp, #20 ... >> ldmfd sp, {r3, r4, fp, sp, pc} @ XXX assert def_cfa->reg is IP >> instead of SP, >> >> Fixed by inserting a REG_CFA_DEF_CFA note, fixing the arm_unwind_emit >> machinery and setting the FRAME_RELATED_P . The comment says : >> >> /* The INSN is generated in epilogue. It is set as RTX_FRAME_RELATED_P >> to get correct dwarf information for shrink-wrap. We should not >> emit unwind information for it because these are used either for >> pretend arguments or notes to adjust sp and restore registers from >> stack. */ >> >> the testsuite score improves without regression (improvements from -g >> and -fexeptions tests) >> >> === gcc Summary for arm-sim//-mapcs-frame === >> >> # of expected passes 77545 >> # of unexpected failures 31 >> # of unexpected successes 2 >> # of expected failures 172 >> # of unsupported tests 1336 >> >> === g++ Summary for arm-sim//-mapcs-frame === >> >> # of expected passes 50116 >> # of unexpected failures 9 >> # of unexpected successes 3 >> # of expected failures 280 >> # of unsupported tests 1229 >> >> instead of >> >> === gcc Summary for arm-sim//-mapcs-frame === >> >> # of expected passes 77106 >> # of unexpected failures 500 >> # of unexpected successes 2 >> # of expected failures 172 >> # of unresolved testcases 111 >> # of unsupported tests 1336 >> >> === g++ Summary for arm-sim//-mapcs-frame === >> >> # of expected passes 50021 >> # of unexpected failures 136 >> # of unexpected successes 3 >> # of expected failures 280 >> # of unsupported tests 1229 >> >> Comments ? OK for trunk ? >> >> Many thanks >> >>
On Mon, Feb 24, 2014 at 9:11 AM, Christian Bruel <christian.bruel@st.com> wrote: > This patch improves the one sent previously, > (http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01159.html), to fix a few > more failures in the testsuite that could arise with shrink-wrap and > -fexceptions. > > To recall, the problem that it fixes is that with -mapcs-frame : > > - the epilogue pops as > > sub sp, fp, #12 @ does not set FRAME_RELATED_P > ldmia sp, {fp, sp, lr} @ XXX assert def_cfa->reg is FP instead > of SP > > - with vrp this is worse, we have > > fldmfdd ip!, {d8} @ FRAME_RELATED_P > sub sp, fp, #20 ... > ldmfd sp, {r3, r4, fp, sp, pc} @ XXX assert def_cfa->reg is IP > instead of SP, > > Fixed by inserting a REG_CFA_DEF_CFA note, fixing the arm_unwind_emit > machinery and setting the FRAME_RELATED_P . The comment says : > > /* The INSN is generated in epilogue. It is set as RTX_FRAME_RELATED_P > to get correct dwarf information for shrink-wrap. We should not > emit unwind information for it because these are used either for > pretend arguments or notes to adjust sp and restore registers from > stack. */ > > the testsuite score improves without regression (improvements from -g > and -fexeptions tests) > > === gcc Summary for arm-sim//-mapcs-frame === > > # of expected passes 77545 > # of unexpected failures 31 > # of unexpected successes 2 > # of expected failures 172 > # of unsupported tests 1336 > > === g++ Summary for arm-sim//-mapcs-frame === > > # of expected passes 50116 > # of unexpected failures 9 > # of unexpected successes 3 > # of expected failures 280 > # of unsupported tests 1229 > > instead of > > === gcc Summary for arm-sim//-mapcs-frame === > > # of expected passes 77106 > # of unexpected failures 500 > # of unexpected successes 2 > # of expected failures 172 > # of unresolved testcases 111 > # of unsupported tests 1336 > > === g++ Summary for arm-sim//-mapcs-frame === > > # of expected passes 50021 > # of unexpected failures 136 > # of unexpected successes 3 > # of expected failures 280 > # of unsupported tests 1229 > > Comments ? OK for trunk ? > > Many thanks > > Please respin using plus_constant instead of gen_addsi3. Otherwise this looks good to me. Please repost updated patch and I will look at it again. Ramana
Hi Ramana, Thanks for your comments, > Please respin using plus_constant instead of gen_addsi3. Here is my feeling about this: I experimented on using plus_constant instead of gen_addsi3. But there are cases when the emitted code is not equivalent for large frames (!const_ok_for_op (val, PLUS)) and leads to complications. We could fix this case with a call to arm_split_constant (PLUS, Pmode, NULL, amount, stack_pointer_rtx, stack_pointer_rtx, 0), but I'm not sure we gain in clarity here. Also for consistency, the same interface change would preferably be needed in the other parts of the arm.c file (that I didn't modify) sharing the same sequence. For instance "arm_expand_epilogue" > Otherwise > this looks good to me. > > Please repost updated patch and I will look at it again. For the reasons expressed above it'd prefer to consider this new change as a separate development with a new patch. For the time being (considering only the original apcs issue) is it OK to apply only the patch as it ? and validate a global gen_addsi3/plus_constant interface review as a separate step ? Many thanks, Christian > > Ramana
On Fri, Mar 7, 2014 at 10:24 AM, Christian Bruel <christian.bruel@st.com> wrote: > Hi Ramana, > > Thanks for your comments, > >> Please respin using plus_constant instead of gen_addsi3. > > Here is my feeling about this: > > I experimented on using plus_constant instead of gen_addsi3. But there > are cases when the emitted code is not equivalent for large frames > (!const_ok_for_op (val, PLUS)) and leads to complications. Ah, Yes you are right. I hadn't remembered that when I looked at it yesterday. > > We could fix this case with a call to arm_split_constant (PLUS, Pmode, > NULL, amount, stack_pointer_rtx, stack_pointer_rtx, 0), but I'm not > sure we gain in clarity here. Also for consistency, the same interface > change would preferably be needed in the other parts of the arm.c file > (that I didn't modify) sharing the same sequence. For instance > "arm_expand_epilogue" > Agreed, There's no need to do that given your explanation. Looks good to me - please give an RM 24 working hours i.e. till end of day on 11th March to comment before committing this. Please also remove mfloat-abi=hard from pr60264.c, this will just conflict when people test for Thumb1 or with other conflicting multilibs i.e. with soft-float. There are enough autotesters with the hardfloat abi these days that we'll find any regressions that might be there. regards Ramana >> Otherwise >> this looks good to me. >> >> Please repost updated patch and I will look at it again. > > For the reasons expressed above it'd prefer to consider this new change > as a separate development with a new patch. > > For the time being (considering only the original apcs issue) is it OK > to apply only the patch as it ? and validate a global > gen_addsi3/plus_constant interface review as a separate step ? > > Many thanks, > > Christian > >> >> Ramana >
2014-02-18 Christian Bruel <christian.bruel@st.com> PR target/60264 * config/arm/arm.c (arm_emit_vfp_multi_reg_pop): Emit a REG_CFA_DEF_CFA note. (arm_expand_epilogue_apcs_frame): call arm_add_cfa_adjust_cfa_note. (arm_unwind_emit): Allow REG_CFA_DEF_CFA. 2014-02-18 Christian Bruel <christian.bruel@st.com> PR target/60264 * gcc.target/arm/pr60264.c * gcc.target/arm/pr60264-2.c Index: gcc/config/arm/arm.c =================================================================== --- gcc/config/arm/arm.c (revision 207942) +++ gcc/config/arm/arm.c (working copy) @@ -19909,8 +19909,15 @@ arm_emit_vfp_multi_reg_pop (int first_reg, int num par = emit_insn (par); REG_NOTES (par) = dwarf; - arm_add_cfa_adjust_cfa_note (par, 2 * UNITS_PER_WORD * num_regs, - base_reg, base_reg); + /* Make sure cfa doesn't leave with IP_REGNUM to allow unwinding fron FP. */ + if (TARGET_VFP && REGNO (base_reg) == IP_REGNUM) + { + RTX_FRAME_RELATED_P (par) = 1; + add_reg_note (par, REG_CFA_DEF_CFA, hard_frame_pointer_rtx); + } + else + arm_add_cfa_adjust_cfa_note (par, 2 * UNITS_PER_WORD * num_regs, + base_reg, base_reg); } /* Generate and emit a pattern that will be recognized as LDRD pattern. If even @@ -27098,15 +27105,19 @@ arm_expand_epilogue_apcs_frame (bool really_return if (TARGET_HARD_FLOAT && TARGET_VFP) { int start_reg; + rtx ip_rtx = gen_rtx_REG (SImode, IP_REGNUM); /* The offset is from IP_REGNUM. */ int saved_size = arm_get_vfp_saved_size (); if (saved_size > 0) { + rtx insn; floats_from_frame += saved_size; - emit_insn (gen_addsi3 (gen_rtx_REG (SImode, IP_REGNUM), - hard_frame_pointer_rtx, - GEN_INT (-floats_from_frame))); + insn = emit_insn (gen_addsi3 (ip_rtx, + hard_frame_pointer_rtx, + GEN_INT (-floats_from_frame))); + arm_add_cfa_adjust_cfa_note (insn, -floats_from_frame, + ip_rtx, hard_frame_pointer_rtx); } /* Generate VFP register multi-pop. */ @@ -27179,11 +27190,15 @@ arm_expand_epilogue_apcs_frame (bool really_return num_regs = bit_count (saved_regs_mask); if ((offsets->outgoing_args != (1 + num_regs)) || cfun->calls_alloca) { + rtx insn; emit_insn (gen_blockage ()); /* Unwind the stack to just below the saved registers. */ - emit_insn (gen_addsi3 (stack_pointer_rtx, - hard_frame_pointer_rtx, - GEN_INT (- 4 * num_regs))); + insn = emit_insn (gen_addsi3 (stack_pointer_rtx, + hard_frame_pointer_rtx, + GEN_INT (- 4 * num_regs))); + + arm_add_cfa_adjust_cfa_note (insn, - 4 * num_regs, + stack_pointer_rtx, hard_frame_pointer_rtx); } arm_emit_multi_reg_pop (saved_regs_mask); @@ -28975,11 +28990,11 @@ arm_unwind_emit (FILE * asm_out_file, rtx insn) emit unwind information for it because these are used either for pretend arguments or notes to adjust sp and restore registers from stack. */ + case REG_CFA_DEF_CFA: case REG_CFA_ADJUST_CFA: case REG_CFA_RESTORE: return; - case REG_CFA_DEF_CFA: case REG_CFA_EXPRESSION: case REG_CFA_OFFSET: /* ??? Only handling here what we actually emit. */ Index: gcc/testsuite/gcc.target/arm/pr60264-2.c =================================================================== --- gcc/testsuite/gcc.target/arm/pr60264-2.c (revision 0) +++ gcc/testsuite/gcc.target/arm/pr60264-2.c (working copy) @@ -0,0 +1,12 @@ +/* { dg-do compile } */ +/* { dg-options "-mapcs -mfloat-abi=hard -g" } */ + +double bar(void); + +int foo(void) +{ + int i = bar() + bar(); + + return i; +} + Index: gcc/testsuite/gcc.target/arm/pr60264.c =================================================================== --- gcc/testsuite/gcc.target/arm/pr60264.c (revision 0) +++ gcc/testsuite/gcc.target/arm/pr60264.c (working copy) @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-mapcs -g" } */ + +void +bar() +{ + foo(); + foo(); +}
This patch improves the one sent previously, (http://gcc.gnu.org/ml/gcc-patches/2014-02/msg01159.html), to fix a few more failures in the testsuite that could arise with shrink-wrap and -fexceptions. To recall, the problem that it fixes is that with -mapcs-frame : - the epilogue pops as sub sp, fp, #12 @ does not set FRAME_RELATED_P ldmia sp, {fp, sp, lr} @ XXX assert def_cfa->reg is FP instead of SP - with vrp this is worse, we have fldmfdd ip!, {d8} @ FRAME_RELATED_P sub sp, fp, #20 ... ldmfd sp, {r3, r4, fp, sp, pc} @ XXX assert def_cfa->reg is IP instead of SP, Fixed by inserting a REG_CFA_DEF_CFA note, fixing the arm_unwind_emit machinery and setting the FRAME_RELATED_P . The comment says : /* The INSN is generated in epilogue. It is set as RTX_FRAME_RELATED_P to get correct dwarf information for shrink-wrap. We should not emit unwind information for it because these are used either for pretend arguments or notes to adjust sp and restore registers from stack. */ the testsuite score improves without regression (improvements from -g and -fexeptions tests) === gcc Summary for arm-sim//-mapcs-frame === # of expected passes 77545 # of unexpected failures 31 # of unexpected successes 2 # of expected failures 172 # of unsupported tests 1336 === g++ Summary for arm-sim//-mapcs-frame === # of expected passes 50116 # of unexpected failures 9 # of unexpected successes 3 # of expected failures 280 # of unsupported tests 1229 instead of === gcc Summary for arm-sim//-mapcs-frame === # of expected passes 77106 # of unexpected failures 500 # of unexpected successes 2 # of expected failures 172 # of unresolved testcases 111 # of unsupported tests 1336 === g++ Summary for arm-sim//-mapcs-frame === # of expected passes 50021 # of unexpected failures 136 # of unexpected successes 3 # of expected failures 280 # of unsupported tests 1229 Comments ? OK for trunk ? Many thanks