Re: [PATCH v2] x86-64: Find a scratch register for large model profiling

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Feb 2, 2024 at 4:22 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Feb 01, 2024 at 03:02:47PM -0800, H.J. Lu wrote:
> > @@ -2763,6 +2789,8 @@ construct_container (machine_mode mode, machine_mode orig_mode,
> >        {
> >        case X86_64_INTEGER_CLASS:
> >        case X86_64_INTEGERSI_CLASS:
> > +     if (!in_return)
> > +       set_int_parameter_registers_bit (intreg[0]);
> >       return gen_rtx_REG (mode, intreg[0]);
> >        case X86_64_SSE_CLASS:
> >        case X86_64_SSEHF_CLASS:
> > @@ -2821,6 +2849,11 @@ construct_container (machine_mode mode, machine_mode orig_mode,
> >        if (mode == BLKmode)
> >       {
> >         /* Use TImode for BLKmode values in 2 integer registers.  */
> > +       if (!in_return)
> > +         {
> > +           set_int_parameter_registers_bit (intreg[0]);
> > +           set_int_parameter_registers_bit (intreg[1]);
> > +         }
> >         exp[0] = gen_rtx_EXPR_LIST (VOIDmode,
> >                                     gen_rtx_REG (TImode, intreg[0]),
> >                                     GEN_INT (0));
>
> Isn't the above (computed from just whether a function has such an argument)
> already available from cum->nregs or similar plus the sequence of argument
> registers?  Or df which certainly also needs to know what registers contain
> arguments?

Fixed in v4 to use df_get_live_out instead.

> Though, the above means a function argument register which isn't used in a
> function will be impossible to use for mcount.
> We certainly can use it (although var-tracking will not know it got
> clobbered).
> So, wouldn't it be better to ask at the start of prologue generation df what
> registers are live at the start of the function (i.e. at the point of the
> NOTE_INSN_PROLOG_END because rest of prologue is emitted before that) and
> remember a suitable register for the profiling there?
> > @@ -22749,6 +22789,38 @@ current_fentry_section (const char **name)
> >    return true;
> >  }
> >
> > +/* Return an caller-saved register, which isn't used for parameter
> > +   passing, at entry for profile.  */
> > +
> > +static int
> > +x86_64_select_profile_regnum (bool r11_ok ATTRIBUTE_UNUSED)
> > +{
> > +  /* Use %r10 if it isn't used by DRAP.  */
> > +  bool r10_ok = !crtl->drap_reg || REGNO (crtl->drap_reg) != R10_REG;
> > +  if (r10_ok)
> > +    return R10_REG;
> > +
> > +  int i;
> > +  for (i = 0; i < FIRST_PSEUDO_REGISTER; i++)
> > +    if (GENERAL_REGNO_P (i)
> > +     && (r10_ok || i != R10_REG)
>
> r10_ok is false here, so just
>   if (flag_fentry != 0 || !crtl->drap_reg || REGNO (crtl->drap_reg) != R10_REG)
>     return R10_REG;
> at the start and don't declare r10_ok.
>
> > +#ifdef NO_PROFILE_COUNTERS
> > +     && (r11_ok || i != R11_REG)
> > +#else
> > +     && i != R11_REG
> > +#endif
> > +     && (!REX2_INT_REGNO_P (i) || TARGET_APX_EGPR)
> > +     && !fixed_regs[i]
> > +     && call_used_regs[i]
> > +     && !test_int_parameter_registers_bit (i))
> > +      return i;
> > +
> > +  sorry ("no register available for profiling %<-mcmodel=large%s%>",
> > +      ix86_cmodel == CM_LARGE_PIC ? " -fPIC" : "");
> > +
> > +  return INVALID_REGNUM;
> > +}
> > +
> >  /* Output assembler code to FILE to increment profiler label # LABELNO
> >     for profiling a function entry.  */
> >  void
> > @@ -22783,42 +22855,60 @@ x86_function_profiler (FILE *file, int labelno ATTRIBUTE_UNUSED)
> >       fprintf (file, "\tleaq\t%sP%d(%%rip), %%r11\n", LPREFIX, labelno);
> >  #endif
> >
> > +      int scratch;
> > +      const char *reg_prefix;
> > +      const char *reg;
> > +
> >        if (!TARGET_PECOFF)
> >       {
> >         switch (ix86_cmodel)
> >           {
> >           case CM_LARGE:
> > -           /* NB: R10 is caller-saved.  Although it can be used as a
> > -              static chain register, it is preserved when calling
> > -              mcount for nested functions.  */
> > +           scratch = x86_64_select_profile_regnum (true);
> > +           reg = hi_reg_name[scratch];
> > +           reg_prefix = LEGACY_INT_REGNO_P (scratch) ? "r" : "";
> >             if (ASSEMBLER_DIALECT == ASM_INTEL)
> > -             fprintf (file, "1:\tmovabs\tr10, OFFSET FLAT:%s\n"
> > -                            "\tcall\tr10\n", mcount_name);
> > +             fprintf (file,
> > +                      "1:\tmovabs\t%s%s, OFFSET FLAT:%s\n"
> > +                      "\tcall\t%s%s\n",
> > +                      reg_prefix, reg, mcount_name, reg_prefix, reg);
> >             else
> > -             fprintf (file, "1:\tmovabsq\t$%s, %%r10\n\tcall\t*%%r10\n",
> > -                      mcount_name);
> > +             fprintf (file,
> > +                      "1:\tmovabsq\t$%s, %%%s%s\n\tcall\t*%%%s%s\n",
> > +                      mcount_name, reg_prefix, reg, reg_prefix, reg);
> >             break;
> >           case CM_LARGE_PIC:
> >  #ifdef NO_PROFILE_COUNTERS
> > +           scratch = x86_64_select_profile_regnum (false);
> > +           reg = hi_reg_name[scratch];
> > +           reg_prefix = LEGACY_INT_REGNO_P (scratch) ? "r" : "";
> >             if (ASSEMBLER_DIALECT == ASM_INTEL)
> >               {
> >                 fprintf (file, "1:movabs\tr11, "
> >                                "OFFSET FLAT:_GLOBAL_OFFSET_TABLE_-1b\n");
> > -               fprintf (file, "\tlea\tr10, 1b[rip]\n");
> > -               fprintf (file, "\tadd\tr10, r11\n");
> > +               fprintf (file, "\tlea\t%s%s, 1b[rip]\n",
> > +                        reg_prefix, reg);
> > +               fprintf (file, "\tadd\t%s%s, r11\n",
> > +                        reg_prefix, reg);
> >                 fprintf (file, "\tmovabs\tr11, OFFSET FLAT:%s@PLTOFF\n",
> >                          mcount_name);
> > -               fprintf (file, "\tadd\tr10, r11\n");
> > -               fprintf (file, "\tcall\tr10\n");
> > +               fprintf (file, "\tadd\t%s%s, r11\n",
> > +                        reg_prefix, reg);
> > +               fprintf (file, "\tcall\t%s%s\n",
> > +                        reg_prefix, reg);
> >                 break;
> >               }
> >             fprintf (file,
> >                      "1:\tmovabsq\t$_GLOBAL_OFFSET_TABLE_-1b, %%r11\n");
> > -           fprintf (file, "\tleaq\t1b(%%rip), %%r10\n");
> > -           fprintf (file, "\taddq\t%%r11, %%r10\n");
> > +           fprintf (file, "\tleaq\t1b(%%rip), %%%s%s\n",
> > +                    reg_prefix, reg);
> > +           fprintf (file, "\taddq\t%%r11, %%%s%s\n",
> > +                    reg_prefix, reg);
> >             fprintf (file, "\tmovabsq\t$%s@PLTOFF, %%r11\n", mcount_name);
> > -           fprintf (file, "\taddq\t%%r11, %%r10\n");
> > -           fprintf (file, "\tcall\t*%%r10\n");
> > +           fprintf (file, "\taddq\t%%r11, %%%s%s\n",
> > +                    reg_prefix, reg);
> > +           fprintf (file, "\tcall\t*%%%s%s\n",
> > +                    reg_prefix, reg);
> >  #else
> >             sorry ("profiling %<-mcmodel=large%> with PIC is not supported");
> >  #endif
> > diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> > index 35ce8b00d36..7967bc5196c 100644
> > --- a/gcc/config/i386/i386.h
> > +++ b/gcc/config/i386/i386.h
> > @@ -2847,6 +2847,11 @@ struct GTY(()) machine_function {
> >    /* True if red zone is used.  */
> >    BOOL_BITFIELD red_zone_used : 1;
> >
> > +  /* Bit mask for integer registers used for parameter passing.
> > +     The lower 8 bits are for legacy registers and the upper
> > +     8 bits are for r8-r15.  */
> > +  unsigned int int_parameter_registers : 16;
> > +
> >    /* The largest alignment, in bytes, of stack slot actually used.  */
> >    unsigned int max_used_stack_alignment;
> >
> > diff --git a/gcc/testsuite/gcc.target/i386/pr113689-1.c b/gcc/testsuite/gcc.target/i386/pr113689-1.c
> > new file mode 100644
> > index 00000000000..c32445e0fc4
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr113689-1.c
> > @@ -0,0 +1,49 @@
> > +/* { dg-do run { target { lp64 && fpic } } } */
> > +/* { dg-options "-O2 -fno-pic -fprofile -mcmodel=large" } */
> > +
> > +#include <stdarg.h>
> > +
> > +__attribute__((noipa,noclone,noinline))
> > +void
> > +bar (int a1, int a2, int a3, int a4, int a5, int a6,
> > +     char *x, char *y, int *z)
> > +{
> > +  if (a1 != 1)
> > +    __builtin_abort ();
> > +  if (a2 != 2)
> > +    __builtin_abort ();
> > +  if (a3 != 3)
> > +    __builtin_abort ();
> > +  if (a4 != 4)
> > +    __builtin_abort ();
> > +  if (a5 != 5)
> > +    __builtin_abort ();
> > +  if (a6 != 6)
> > +    __builtin_abort ();
> > +  x[0] = 42;
> > +  y[0] = 42;
> > +  if (z[0] != 16)
> > +    __builtin_abort ();
> > +}
> > +
> > +__attribute__((noipa,noclone,noinline))
> > +void
> > +foo (int c, int d, int e, int f, int g, int h, int z, ...)
> > +{
> > +  typedef char B[32];
> > +  B b __attribute__((aligned (32)));
> > +  va_list ap;
> > +  va_start (ap, z);
> > +  int x = va_arg (ap, int);
> > +  if (x != 38)
> > +    __builtin_abort ();
> > +  bar (c, d, e, f, g, h, &b[0], __builtin_alloca (z), &z);
> > +  va_end (ap);
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  foo (1, 2, 3, 4, 5, 6, 16, 38);
> > +  return 0;
> > +}
> > diff --git a/gcc/testsuite/gcc.target/i386/pr113689-2.c b/gcc/testsuite/gcc.target/i386/pr113689-2.c
> > new file mode 100644
> > index 00000000000..fec5b171a39
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr113689-2.c
> > @@ -0,0 +1,41 @@
> > +/* { dg-do run { target { lp64 && fpic } } } */
> > +/* { dg-options "-O2 -fpic -fprofile -mcmodel=large" } */
> > +
> > +__attribute__((noipa,noclone,noinline))
> > +void
> > +bar (int a1, int a2, int a3, int a4, int a5, int a6,
> > +     char *x, char *y, int *z)
> > +{
> > +  if (a1 != 1)
> > +    __builtin_abort ();
> > +  if (a2 != 2)
> > +    __builtin_abort ();
> > +  if (a3 != 3)
> > +    __builtin_abort ();
> > +  if (a4 != 4)
> > +    __builtin_abort ();
> > +  if (a5 != 5)
> > +    __builtin_abort ();
> > +  if (a6 != 6)
> > +    __builtin_abort ();
> > +  x[0] = 42;
> > +  y[0] = 42;
> > +  if (z[0] != 16)
> > +    __builtin_abort ();
> > +}
> > +
> > +__attribute__((noipa,noclone,noinline))
> > +void
> > +foo (int c, int d, int e, int f, int g, int h, int z)
> > +{
> > +  typedef char B[32];
> > +  B b __attribute__((aligned (32)));
> > +  bar (c, d, e, f, g, h, &b[0], __builtin_alloca (z), &z);
> > +}
> > +
> > +int
> > +main ()
> > +{
> > +  foo (1, 2, 3, 4, 5, 6, 16);
> > +  return 0;
> > +}
> > diff --git a/gcc/testsuite/gcc.target/i386/pr113689-3.c b/gcc/testsuite/gcc.target/i386/pr113689-3.c
> > new file mode 100644
> > index 00000000000..10099fc8d96
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/pr113689-3.c
> > @@ -0,0 +1,24 @@
> > +/* { dg-do run { target { lp64 && avx_runtime } } } */
>
> I think this test isn't needed.
> If you really want it, it should have bitint effective
> target and guard with __BITINT_MAXWIDTH__ >= 511 to be
> consistent with other bitint tests.

Removed in v4.

> > +/* { dg-options "-O2 -fprofile -mcmodel=large -mavx" } */
> > +
> > +__attribute__((noipa,noclone,noinline))
> > +_BitInt(511)
> > +foo(_BitInt(7) a, _BitInt(511) b)
> > +{
> > +  ({
> > +    volatile _BitInt(511) d = (-b / a);
> > +    if (d != -1)
> > +      __builtin_abort();
> > +    d;
> > +  });
> > +  return b;
> > +}
> > +
> > +int
> > +main(void)
> > +{
> > +  _BitInt(511) x = foo(5, 5);
> > +  if (x != 5)
> > +    __builtin_abort();
> > +  return 0;
> > +}
> > --
> > 2.43.0
>
>         Jakub
>


-- 
H.J.