Re: [PATCH #1/2] strub: handle volatile promoted args in internal strub [PR112938]

[Richard Biener <richard.guenther@gmail.com>]

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