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

  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
  

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