On Tue, Dec 12, 2023 at 3:03 AM Alexandre Oliva <ol...@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
