| Message ID | orlea06uut.fsf@lxoliva.fsfla.org |
|---|---|
| State | New |
| Headers | show |
| Series | [#1/2] strub: handle volatile promoted args in internal strub [PR112938] | expand |
On Tue, Dec 12, 2023 at 3:03 AM Alexandre Oliva <oliva@adacore.com> wrote: > > > When generating code for an internal strub wrapper, don't clear the > DECL_NOT_GIMPLE_REG_P flag of volatile args, and gimplify them both > before and after any conversion. > > While at that, move variable TMP into narrower scopes so that it's > more trivial to track where ARG lives. > > Regstrapped on x86_64-linux-gnu. Ok to install? > > (there's a #2/2 followup coming up that addresses the ??? comment added > herein) > > > for gcc/ChangeLog > > PR middle-end/112938 > * ipa-strub.cc (pass_ipa_strub::execute): Handle promoted > volatile args in internal strub. Simplify. > > for gcc/testsuite/ChangeLog > > PR middle-end/112938 > * gcc.dg/strub-internal-volatile.c: New. > --- > gcc/ipa-strub.cc | 29 +++++++++++++++++------- > gcc/testsuite/gcc.dg/strub-internal-volatile.c | 10 ++++++++ > 2 files changed, 31 insertions(+), 8 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/strub-internal-volatile.c > > diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc > index 8ec6824e8a802..45294b0b46bcb 100644 > --- a/gcc/ipa-strub.cc > +++ b/gcc/ipa-strub.cc > @@ -3203,7 +3203,6 @@ pass_ipa_strub::execute (function *) > i++, arg = DECL_CHAIN (arg), nparm = DECL_CHAIN (nparm)) > { > tree save_arg = arg; > - tree tmp = arg; > > /* Arrange to pass indirectly the parms, if we decided to do > so, and revert its type in the wrapper. */ > @@ -3211,10 +3210,9 @@ pass_ipa_strub::execute (function *) > { > tree ref_type = TREE_TYPE (nparm); > TREE_ADDRESSABLE (arg) = true; > - tree addr = build1 (ADDR_EXPR, ref_type, arg); > - tmp = arg = addr; > + arg = build1 (ADDR_EXPR, ref_type, arg); > } > - else > + else if (!TREE_THIS_VOLATILE (arg)) > DECL_NOT_GIMPLE_REG_P (arg) = 0; I wonder why you clear this at all? The next update_address_taken will take care of this if possible. > > /* Convert the argument back to the type used by the calling > @@ -3223,16 +3221,31 @@ pass_ipa_strub::execute (function *) > double to be passed on unchanged to the wrapped > function. */ > if (TREE_TYPE (nparm) != DECL_ARG_TYPE (nparm)) > - arg = fold_convert (DECL_ARG_TYPE (nparm), arg); > + { > + tree tmp = arg; > + /* If ARG is e.g. volatile, we must copy and > + convert in separate statements. ??? Should > + we drop volatile from the wrapper > + instead? */ volatile on function parameters are indeed odd beasts. You could also force volatile arguments to be passed indirectly. I think for GIMPLE thunks we do it as you now do here. > + if (!is_gimple_val (arg)) > + { > + tmp = create_tmp_reg (TYPE_MAIN_VARIANT > + (TREE_TYPE (arg)), "arg"); > + gimple *stmt = gimple_build_assign (tmp, arg); > + gsi_insert_after (&bsi, stmt, GSI_NEW_STMT); > + } > + arg = fold_convert (DECL_ARG_TYPE (nparm), tmp); > + } > > if (!is_gimple_val (arg)) > { > - tmp = create_tmp_reg (TYPE_MAIN_VARIANT > - (TREE_TYPE (arg)), "arg"); > + tree tmp = create_tmp_reg (TYPE_MAIN_VARIANT > + (TREE_TYPE (arg)), "arg"); > gimple *stmt = gimple_build_assign (tmp, arg); > gsi_insert_after (&bsi, stmt, GSI_NEW_STMT); > + arg = tmp; > } > - vargs.quick_push (tmp); > + vargs.quick_push (arg); > arg = save_arg; > } > /* These strub arguments are adjusted later. */ > diff --git a/gcc/testsuite/gcc.dg/strub-internal-volatile.c b/gcc/testsuite/gcc.dg/strub-internal-volatile.c > new file mode 100644 > index 0000000000000..cdfca67616bc8 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/strub-internal-volatile.c > @@ -0,0 +1,10 @@ > +/* { dg-do compile } */ > +/* { dg-require-effective-target strub } */ > + > +void __attribute__ ((strub("internal"))) > +f(volatile short) { > +} > + > +void g(void) { > + f(0); > +} > > -- > Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ > Free Software Activist GNU Toolchain Engineer > More tolerance and less prejudice are key for inclusion and diversity > Excluding neuro-others for not behaving ""normal"" is *not* inclusive
diff --git a/gcc/ipa-strub.cc b/gcc/ipa-strub.cc index 8ec6824e8a802..45294b0b46bcb 100644 --- a/gcc/ipa-strub.cc +++ b/gcc/ipa-strub.cc @@ -3203,7 +3203,6 @@ pass_ipa_strub::execute (function *) i++, arg = DECL_CHAIN (arg), nparm = DECL_CHAIN (nparm)) { tree save_arg = arg; - tree tmp = arg; /* Arrange to pass indirectly the parms, if we decided to do so, and revert its type in the wrapper. */ @@ -3211,10 +3210,9 @@ pass_ipa_strub::execute (function *) { tree ref_type = TREE_TYPE (nparm); TREE_ADDRESSABLE (arg) = true; - tree addr = build1 (ADDR_EXPR, ref_type, arg); - tmp = arg = addr; + arg = build1 (ADDR_EXPR, ref_type, arg); } - else + else if (!TREE_THIS_VOLATILE (arg)) DECL_NOT_GIMPLE_REG_P (arg) = 0; /* Convert the argument back to the type used by the calling @@ -3223,16 +3221,31 @@ pass_ipa_strub::execute (function *) double to be passed on unchanged to the wrapped function. */ if (TREE_TYPE (nparm) != DECL_ARG_TYPE (nparm)) - arg = fold_convert (DECL_ARG_TYPE (nparm), arg); + { + tree tmp = arg; + /* If ARG is e.g. volatile, we must copy and + convert in separate statements. ??? Should + we drop volatile from the wrapper + instead? */ + if (!is_gimple_val (arg)) + { + tmp = create_tmp_reg (TYPE_MAIN_VARIANT + (TREE_TYPE (arg)), "arg"); + gimple *stmt = gimple_build_assign (tmp, arg); + gsi_insert_after (&bsi, stmt, GSI_NEW_STMT); + } + arg = fold_convert (DECL_ARG_TYPE (nparm), tmp); + } if (!is_gimple_val (arg)) { - tmp = create_tmp_reg (TYPE_MAIN_VARIANT - (TREE_TYPE (arg)), "arg"); + tree tmp = create_tmp_reg (TYPE_MAIN_VARIANT + (TREE_TYPE (arg)), "arg"); gimple *stmt = gimple_build_assign (tmp, arg); gsi_insert_after (&bsi, stmt, GSI_NEW_STMT); + arg = tmp; } - vargs.quick_push (tmp); + vargs.quick_push (arg); arg = save_arg; } /* These strub arguments are adjusted later. */ diff --git a/gcc/testsuite/gcc.dg/strub-internal-volatile.c b/gcc/testsuite/gcc.dg/strub-internal-volatile.c new file mode 100644 index 0000000000000..cdfca67616bc8 --- /dev/null +++ b/gcc/testsuite/gcc.dg/strub-internal-volatile.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-require-effective-target strub } */ + +void __attribute__ ((strub("internal"))) +f(volatile short) { +} + +void g(void) { + f(0); +}