| Message ID | 80d76433-7478-d3b3-f656-e13f15b1b7b4@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Don't clobber return value when eh_return called [PR114846] | expand |
On Thu, May 16, 2024, 4:09 AM Kewen.Lin <linkw@linux.ibm.com> wrote: > Hi, > > As the associated test case in PR114846 shows, currently > with eh_return involved some register restoring for EH > RETURN DATA in epilogue can clobber the one which holding > the return value. Referring to the existing handlings in > some other targets, this patch makes eh_return expander > call one new define_insn_and_split eh_return_internal which > directly calls rs6000_emit_epilogue with epilogue_type > EPILOGUE_TYPE_EH_RETURN instead of the previous treating > normal return with crtl->calls_eh_return specially. > > Bootstrapped and regtested on powerpc64-linux-gnu P8/P9 and > powerpc64le-linux-gnu P9 and P10. > > I'm going to push this next week if no objections. > Thanks for fixing this for powerpc. I hope my patch for aarch64 gets reviewed soon and it will contain many more testcases. Hopefully someone will fix the arm target too. Thanks, Andrew > BR, > Kewen > ----- > PR target/114846 > > gcc/ChangeLog: > > * config/rs6000/rs6000-logue.cc (rs6000_emit_epilogue): As > EPILOGUE_TYPE_EH_RETURN would be passed as epilogue_type directly > now, adjust the relevant handlings on it. > * config/rs6000/rs6000.md (eh_return expander): Append by calling > gen_eh_return_internal and emit_barrier. > (eh_return_internal): New define_insn_and_split, call function > rs6000_emit_epilogue with epilogue type EPILOGUE_TYPE_EH_RETURN. > > gcc/testsuite/ChangeLog: > > * gcc.target/powerpc/pr114846.c: New test. > --- > gcc/config/rs6000/rs6000-logue.cc | 7 +++---- > gcc/config/rs6000/rs6000.md | 15 +++++++++++++++ > gcc/testsuite/gcc.target/powerpc/pr114846.c | 20 ++++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr114846.c > > diff --git a/gcc/config/rs6000/rs6000-logue.cc > b/gcc/config/rs6000/rs6000-logue.cc > index 60ba15a8bc3..bd5d56ba002 100644 > --- a/gcc/config/rs6000/rs6000-logue.cc > +++ b/gcc/config/rs6000/rs6000-logue.cc > @@ -4308,9 +4308,6 @@ rs6000_emit_epilogue (enum epilogue_type > epilogue_type) > > rs6000_stack_t *info = rs6000_stack_info (); > > - if (epilogue_type == EPILOGUE_TYPE_NORMAL && crtl->calls_eh_return) > - epilogue_type = EPILOGUE_TYPE_EH_RETURN; > - > int strategy = info->savres_strategy; > bool using_load_multiple = !!(strategy & REST_MULTIPLE); > bool restoring_GPRs_inline = !!(strategy & REST_INLINE_GPRS); > @@ -4788,7 +4785,9 @@ rs6000_emit_epilogue (enum epilogue_type > epilogue_type) > > /* In the ELFv2 ABI we need to restore all call-saved CR fields from > *separate* slots if the routine calls __builtin_eh_return, so > - that they can be independently restored by the unwinder. */ > + that they can be independently restored by the unwinder. Since > + it is for CR fields restoring, it should be done for any epilogue > + types (not EPILOGUE_TYPE_EH_RETURN specific). */ > if (DEFAULT_ABI == ABI_ELFv2 && crtl->calls_eh_return) > { > int i, cr_off = info->ehcr_offset; > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md > index ac5651d7420..d4120c3b9ce 100644 > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -14281,6 +14281,8 @@ (define_expand "eh_return" > "" > { > emit_insn (gen_eh_set_lr (Pmode, operands[0])); > + emit_jump_insn (gen_eh_return_internal ()); > + emit_barrier (); > DONE; > }) > > @@ -14297,6 +14299,19 @@ (define_insn_and_split "@eh_set_lr_<mode>" > DONE; > }) > > +(define_insn_and_split "eh_return_internal" > + [(eh_return)] > + "" > + "#" > + "epilogue_completed" > + [(const_int 0)] > +{ > + if (!TARGET_SCHED_PROLOG) > + emit_insn (gen_blockage ()); > + rs6000_emit_epilogue (EPILOGUE_TYPE_EH_RETURN); > + DONE; > +}) > + > (define_insn "prefetch" > [(prefetch (match_operand 0 "indexed_or_indirect_address" "a") > (match_operand:SI 1 "const_int_operand" "n") > diff --git a/gcc/testsuite/gcc.target/powerpc/pr114846.c > b/gcc/testsuite/gcc.target/powerpc/pr114846.c > new file mode 100644 > index 00000000000..efe2300b73a > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr114846.c > @@ -0,0 +1,20 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target builtin_eh_return } */ > + > +/* Ensure it runs successfully. */ > + > +__attribute__ ((noipa)) > +int f (int *a, long offset, void *handler) > +{ > + if (*a == 5) > + return 5; > + __builtin_eh_return (offset, handler); > +} > + > +int main () > +{ > + int t = 5; > + if (f (&t, 0, 0) != 5) > + __builtin_abort (); > + return 0; > +} > -- > 2.39.3 >
Hi, on 2024/5/16 12:08, Andrew Pinski wrote: > > On Thu, May 16, 2024, 4:09 AM Kewen.Lin <linkw@linux.ibm.com <mailto:linkw@linux.ibm.com>> wrote: > > Hi, > > As the associated test case in PR114846 shows, currently > with eh_return involved some register restoring for EH > RETURN DATA in epilogue can clobber the one which holding > the return value. Referring to the existing handlings in > some other targets, this patch makes eh_return expander > call one new define_insn_and_split eh_return_internal which > directly calls rs6000_emit_epilogue with epilogue_type > EPILOGUE_TYPE_EH_RETURN instead of the previous treating > normal return with crtl->calls_eh_return specially. > > Bootstrapped and regtested on powerpc64-linux-gnu P8/P9 and > powerpc64le-linux-gnu P9 and P10. > > I'm going to push this next week if no objections. > > > > Thanks for fixing this for powerpc. I hope my patch for aarch64 gets reviewed soon and it will contain many more testcases. Hopefully someone will fix the arm target too. > Looking forward to that! Thanks for contributing those new eh-return c-torture test cases, I just tested all of them on LE, all passed. :) BR, Kewen > Thanks, > Andrew > > > > BR, > Kewen > ----- > PR target/114846 > > gcc/ChangeLog: > > * config/rs6000/rs6000-logue.cc (rs6000_emit_epilogue): As > EPILOGUE_TYPE_EH_RETURN would be passed as epilogue_type directly > now, adjust the relevant handlings on it. > * config/rs6000/rs6000.md (eh_return expander): Append by calling > gen_eh_return_internal and emit_barrier. > (eh_return_internal): New define_insn_and_split, call function > rs6000_emit_epilogue with epilogue type EPILOGUE_TYPE_EH_RETURN. > > gcc/testsuite/ChangeLog: > > * gcc.target/powerpc/pr114846.c: New test. > --- > gcc/config/rs6000/rs6000-logue.cc | 7 +++---- > gcc/config/rs6000/rs6000.md | 15 +++++++++++++++ > gcc/testsuite/gcc.target/powerpc/pr114846.c | 20 ++++++++++++++++++++ > 3 files changed, 38 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/powerpc/pr114846.c > > diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc > index 60ba15a8bc3..bd5d56ba002 100644 > --- a/gcc/config/rs6000/rs6000-logue.cc > +++ b/gcc/config/rs6000/rs6000-logue.cc > @@ -4308,9 +4308,6 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) > > rs6000_stack_t *info = rs6000_stack_info (); > > - if (epilogue_type == EPILOGUE_TYPE_NORMAL && crtl->calls_eh_return) > - epilogue_type = EPILOGUE_TYPE_EH_RETURN; > - > int strategy = info->savres_strategy; > bool using_load_multiple = !!(strategy & REST_MULTIPLE); > bool restoring_GPRs_inline = !!(strategy & REST_INLINE_GPRS); > @@ -4788,7 +4785,9 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) > > /* In the ELFv2 ABI we need to restore all call-saved CR fields from > *separate* slots if the routine calls __builtin_eh_return, so > - that they can be independently restored by the unwinder. */ > + that they can be independently restored by the unwinder. Since > + it is for CR fields restoring, it should be done for any epilogue > + types (not EPILOGUE_TYPE_EH_RETURN specific). */ > if (DEFAULT_ABI == ABI_ELFv2 && crtl->calls_eh_return) > { > int i, cr_off = info->ehcr_offset; > diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md > index ac5651d7420..d4120c3b9ce 100644 > --- a/gcc/config/rs6000/rs6000.md > +++ b/gcc/config/rs6000/rs6000.md > @@ -14281,6 +14281,8 @@ (define_expand "eh_return" > "" > { > emit_insn (gen_eh_set_lr (Pmode, operands[0])); > + emit_jump_insn (gen_eh_return_internal ()); > + emit_barrier (); > DONE; > }) > > @@ -14297,6 +14299,19 @@ (define_insn_and_split "@eh_set_lr_<mode>" > DONE; > }) > > +(define_insn_and_split "eh_return_internal" > + [(eh_return)] > + "" > + "#" > + "epilogue_completed" > + [(const_int 0)] > +{ > + if (!TARGET_SCHED_PROLOG) > + emit_insn (gen_blockage ()); > + rs6000_emit_epilogue (EPILOGUE_TYPE_EH_RETURN); > + DONE; > +}) > + > (define_insn "prefetch" > [(prefetch (match_operand 0 "indexed_or_indirect_address" "a") > (match_operand:SI 1 "const_int_operand" "n") > diff --git a/gcc/testsuite/gcc.target/powerpc/pr114846.c b/gcc/testsuite/gcc.target/powerpc/pr114846.c > new file mode 100644 > index 00000000000..efe2300b73a > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr114846.c > @@ -0,0 +1,20 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target builtin_eh_return } */ > + > +/* Ensure it runs successfully. */ > + > +__attribute__ ((noipa)) > +int f (int *a, long offset, void *handler) > +{ > + if (*a == 5) > + return 5; > + __builtin_eh_return (offset, handler); > +} > + > +int main () > +{ > + int t = 5; > + if (f (&t, 0, 0) != 5) > + __builtin_abort (); > + return 0; > +} > -- > 2.39.3 >
diff --git a/gcc/config/rs6000/rs6000-logue.cc b/gcc/config/rs6000/rs6000-logue.cc index 60ba15a8bc3..bd5d56ba002 100644 --- a/gcc/config/rs6000/rs6000-logue.cc +++ b/gcc/config/rs6000/rs6000-logue.cc @@ -4308,9 +4308,6 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) rs6000_stack_t *info = rs6000_stack_info (); - if (epilogue_type == EPILOGUE_TYPE_NORMAL && crtl->calls_eh_return) - epilogue_type = EPILOGUE_TYPE_EH_RETURN; - int strategy = info->savres_strategy; bool using_load_multiple = !!(strategy & REST_MULTIPLE); bool restoring_GPRs_inline = !!(strategy & REST_INLINE_GPRS); @@ -4788,7 +4785,9 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type) /* In the ELFv2 ABI we need to restore all call-saved CR fields from *separate* slots if the routine calls __builtin_eh_return, so - that they can be independently restored by the unwinder. */ + that they can be independently restored by the unwinder. Since + it is for CR fields restoring, it should be done for any epilogue + types (not EPILOGUE_TYPE_EH_RETURN specific). */ if (DEFAULT_ABI == ABI_ELFv2 && crtl->calls_eh_return) { int i, cr_off = info->ehcr_offset; diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md index ac5651d7420..d4120c3b9ce 100644 --- a/gcc/config/rs6000/rs6000.md +++ b/gcc/config/rs6000/rs6000.md @@ -14281,6 +14281,8 @@ (define_expand "eh_return" "" { emit_insn (gen_eh_set_lr (Pmode, operands[0])); + emit_jump_insn (gen_eh_return_internal ()); + emit_barrier (); DONE; }) @@ -14297,6 +14299,19 @@ (define_insn_and_split "@eh_set_lr_<mode>" DONE; }) +(define_insn_and_split "eh_return_internal" + [(eh_return)] + "" + "#" + "epilogue_completed" + [(const_int 0)] +{ + if (!TARGET_SCHED_PROLOG) + emit_insn (gen_blockage ()); + rs6000_emit_epilogue (EPILOGUE_TYPE_EH_RETURN); + DONE; +}) + (define_insn "prefetch" [(prefetch (match_operand 0 "indexed_or_indirect_address" "a") (match_operand:SI 1 "const_int_operand" "n") diff --git a/gcc/testsuite/gcc.target/powerpc/pr114846.c b/gcc/testsuite/gcc.target/powerpc/pr114846.c new file mode 100644 index 00000000000..efe2300b73a --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr114846.c @@ -0,0 +1,20 @@ +/* { dg-do run } */ +/* { dg-require-effective-target builtin_eh_return } */ + +/* Ensure it runs successfully. */ + +__attribute__ ((noipa)) +int f (int *a, long offset, void *handler) +{ + if (*a == 5) + return 5; + __builtin_eh_return (offset, handler); +} + +int main () +{ + int t = 5; + if (f (&t, 0, 0) != 5) + __builtin_abort (); + return 0; +}