Re: [PATCH] expr.cc: avoid unexpected side effects in expand_expr_divmod optimization

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

On Wed, Jan 4, 2023 at 9:54 AM Jose E. Marchesi via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> ping.
> Would this be a good approach for fixing the issue?

adding the is_libcall bit enlarges rtx_def by 8 bytes - there's no room for more
bits here.

I really wonder how other targets avoid the issue you are pointing out?
Do their assemblers prune unused (extern) .global?

> > Hi Jakub.
> >
> >> On Thu, Dec 08, 2022 at 02:02:36PM +0100, Jose E. Marchesi wrote:
> >>> So, I guess the right fix would be to call assemble_external_libcall
> >>> during final?  The `.global FOO' directive would be generated
> >>> immediately before the call sequence, but I guess that would be ok.
> >>
> >> During final only if all the targets can deal with the effects of
> >> assemble_external_libcall being done in the middle of emitting assembly
> >> for the function.
> >>
> >> Otherwise, it could be e.g. done in the first loop of shorten_branches.
> >>
> >> Note, in calls.cc it is done only for emit_library_call_value_1
> >> and not for emit_call_1, so if we do it late, we need to be able to find
> >> out what call is to a libcall and what is to a normal call.  If there is
> >> no way to differentiate it right now, perhaps we need some flag somewhere,
> >> say on a SYMBOL_REF.  And then assemble_external_libcall either only
> >> if such a SYMBOL_REF appears in CALL_INSN or sibcall JUMP_INSN, or
> >> perhaps anywhere in the function and its constant pool.
> >
> > Allright, the quick-and-dirty patch below seems to DTRT with simple
> > examples.
> >
> > First, when libcalls are generated.  Note only one .global is generated
> > for all calls, and actually it is around the same position than before:
> >
> >   $ cat foo.c
> >   int foo(unsigned int len, int flag)
> >   {
> >     if (flag)
> >       return (((long)len) * 234 / 5);
> >     return (((long)len) * 2 / 5);
> >   }
> >   $ cc1 -O2 foo.c
> >   $ cat foo.c
> >       .file   "foo.c"
> >       .text
> >       .global __divdi3
> >       .align  3
> >       .global foo
> >       .type   foo, @function
> >   foo:
> >       mov32   %r1,%r1
> >       lsh     %r2,32
> >       jne     %r2,0,.L5
> >       mov     %r2,5
> >       lsh     %r1,1
> >       call    __divdi3
> >       lsh     %r0,32
> >       arsh    %r0,32
> >       exit
> >   .L5:
> >       mov     %r2,5
> >       mul     %r1,234
> >       call    __divdi3
> >       lsh     %r0,32
> >       arsh    %r0,32
> >       exit
> >       .size   foo, .-foo
> >       .ident  "GCC: (GNU) 13.0.0 20221207 (experimental)"
> >
> > Second, when libcalls are tried by expand_moddiv in a sequence, but then
> > discarded and not linked in the main sequence:
> >
> >   $ cat foo.c
> >   int foo(unsigned int len, int flag)
> >   {
> >     if (flag)
> >       return (((long)len) * 234 / 5);
> >     return (((long)len) * 2 / 5);
> >   }
> >   $ cc1 -O2 foo.c
> >   $ cat foo.c
> >       .file   "foo.c"
> >       .text
> >       .align  3
> >       .global foo
> >       .type   foo, @function
> >   foo:
> >       mov32   %r0,%r1
> >       lsh     %r2,32
> >       jne     %r2,0,.L5
> >       add     %r0,%r0
> >       div     %r0,5
> >       lsh     %r0,32
> >       arsh    %r0,32
> >       exit
> >   .L5:
> >       mul     %r0,234
> >       div     %r0,5
> >       lsh     %r0,32
> >       arsh    %r0,32
> >       exit
> >       .size   foo, .-foo
> >       .ident  "GCC: (GNU) 13.0.0 20221207 (experimental)"
> >
> > Note the .global now is not generated, as desired.
> >
> > As you can see below, I am adding a new RTX flag `is_libcall', with
> > written form "/l".
> >
> > Before I get into serious testing etc, can you please confirm whether
> > this is the right approach or not?
> >
> > In particular, I am a little bit concerned about the expectation I am
> > using that the target of the `call' instruction emitted by emit_call_1
> > is always a (MEM (SYMBOL_REF ...)) when it is passed a SYMBOL_REF as the
> > first argument (`fun' in emit_library_call_value_1).
> >
> > Thanks.
> >
> > diff --git a/gcc/calls.cc b/gcc/calls.cc
> > index 6dd6f73e978..6c4a3725272 100644
> > --- a/gcc/calls.cc
> > +++ b/gcc/calls.cc
> > @@ -4370,10 +4370,6 @@ emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
> >       || argvec[i].partial != 0)
> >        update_stack_alignment_for_call (&argvec[i].locate);
> >
> > -  /* If this machine requires an external definition for library
> > -     functions, write one out.  */
> > -  assemble_external_libcall (fun);
> > -
> >    original_args_size = args_size;
> >    args_size.constant = (aligned_upper_bound (args_size.constant
> >                                            + stack_pointer_delta,
> > @@ -4717,6 +4713,9 @@ emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
> >              valreg,
> >              old_inhibit_defer_pop + 1, call_fusage, flags, args_so_far);
> >
> > +  /* Mark the emitted call as a libcall with the new flag.  */
> > +  RTL_LIBCALL_P (last_call_insn ()) = 1;
> > +
> >    if (flag_ipa_ra)
> >      {
> >        rtx datum = orgfun;
> > diff --git a/gcc/final.cc b/gcc/final.cc
> > index eea572238f6..df57de5afd0 100644
> > --- a/gcc/final.cc
> > +++ b/gcc/final.cc
> > @@ -815,6 +815,8 @@ make_pass_compute_alignments (gcc::context *ctxt)
> >     reorg.cc, since the branch splitting exposes new instructions with delay
> >     slots.  */
> >
> > +static rtx call_from_call_insn (rtx_call_insn *insn);
> > +
> >  void
> >  shorten_branches (rtx_insn *first)
> >  {
> > @@ -850,6 +852,24 @@ shorten_branches (rtx_insn *first)
> >    for (insn = get_insns (), i = 1; insn; insn = NEXT_INSN (insn))
> >      {
> >        INSN_SHUID (insn) = i++;
> > +
> > +      /* If this is a `call' instruction that implements a libcall,
> > +         and this machine requires an external definition for library
> > +         functions, write one out.  */
> > +      if (CALL_P (insn) && RTL_LIBCALL_P (insn))
> > +        {
> > +          rtx nested_call = call_from_call_insn (safe_as_a <rtx_call_insn*> (insn));
> > +          rtx mem = XEXP (nested_call, 0);
> > +          gcc_assert (GET_CODE (mem) == MEM);
> > +          rtx fun = XEXP (mem, 0);
> > +          gcc_assert (GET_CODE (fun) == SYMBOL_REF);
> > +          assemble_external_libcall (fun);
> > +
> > +          /* Clear the LIBCALL flag to make sure we don't assemble the
> > +             external definition more than once.  */
> > +          RTL_LIBCALL_P (insn) = 0;
> > +        }
> > +
> >        if (INSN_P (insn))
> >       continue;
> >
> > diff --git a/gcc/print-rtl.cc b/gcc/print-rtl.cc
> > index e115f987173..26a06511619 100644
> > --- a/gcc/print-rtl.cc
> > +++ b/gcc/print-rtl.cc
> > @@ -882,6 +882,9 @@ rtx_writer::print_rtx (const_rtx in_rtx)
> >        if (RTX_FLAG (in_rtx, return_val))
> >       fputs ("/i", m_outfile);
> >
> > +      if (RTX_FLAG (in_rtx, is_libcall))
> > +        fputs ("/l", m_outfile);
> > +
> >        /* Print REG_NOTE names for EXPR_LIST and INSN_LIST.  */
> >        if ((GET_CODE (in_rtx) == EXPR_LIST
> >          || GET_CODE (in_rtx) == INSN_LIST
> > diff --git a/gcc/read-rtl.cc b/gcc/read-rtl.cc
> > index 62c7895af60..eb0ae150a5b 100644
> > --- a/gcc/read-rtl.cc
> > +++ b/gcc/read-rtl.cc
> > @@ -1543,6 +1543,9 @@ read_flags (rtx return_rtx)
> >         case 'i':
> >           RTX_FLAG (return_rtx, return_val) = 1;
> >           break;
> > +          case 'l':
> > +            RTX_FLAG (return_rtx, is_libcall) = 1;
> > +            break;
> >         default:
> >           fatal_with_file_and_line ("unrecognized flag: `%c'", flag_char);
> >       }
> > diff --git a/gcc/rtl.h b/gcc/rtl.h
> > index 7a8c4709257..92c802d3876 100644
> > --- a/gcc/rtl.h
> > +++ b/gcc/rtl.h
> > @@ -401,6 +401,10 @@ struct GTY((desc("0"), tag("0"),
> >       Dumped as "/i" in RTL dumps.  */
> >    unsigned return_val : 1;
> >
> > +  /* 1 in a CALL_INSN if it is a libcall.
> > +     Dumped as "/l" in RTL dumps.  */
> > +  unsigned is_libcall : 1;
> > +
> >    union {
> >      /* The final union field is aligned to 64 bits on LP64 hosts,
> >         giving a 32-bit gap after the fields above.  We optimize the
> > @@ -1578,6 +1582,10 @@ jump_table_for_label (const rtx_code_label *label)
> >  #define RTL_PURE_CALL_P(RTX)                                 \
> >    (RTL_FLAG_CHECK1 ("RTL_PURE_CALL_P", (RTX), CALL_INSN)->return_val)
> >
> > +/* 1 if RTX is a libcall.  */
> > +#define RTL_LIBCALL_P(RTX)                      \
> > +  (RTL_FLAG_CHECK1 ("RTL_LIBCALL_P", (RTX), CALL_INSN)->is_libcall)
> > +
> >  /* 1 if RTX is a call to a const or pure function.  */
> >  #define RTL_CONST_OR_PURE_CALL_P(RTX) \
> >    (RTL_CONST_CALL_P (RTX) || RTL_PURE_CALL_P (RTX))