Hi Jakub.
Thanks for the review.

> On Fri, Aug 18, 2023 at 03:53:51PM +0200, Jose E. Marchesi via Gcc-patches 
> wrote:
>> --- 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);
>> +
>
> I'd say the forward declaration should go before the function comment, so
> that it is clear the function comment talks about shorten_branches.

Will do.

>
>>  void
>>  shorten_branches (rtx_insn *first)
>>  {
>> @@ -850,6 +852,20 @@ 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 implementing a libcall,
>> +         and this machine requires an external definition for library
>> +         functions, write one out.  */
>> +      if (CALL_P (insn))
>> +        {
>> +          rtx x = call_from_call_insn (dyn_cast <rtx_call_insn *> (insn));
>> +          x = XEXP (x, 0);
>> +          if (x && MEM_P (x)
>
> When all conditions don't fit on one line, each && condition should be on
> its own line.

Will fix.

>
>> +              && SYMBOL_REF_P (XEXP (x, 0))
>> +              && SYMBOL_REF_LIBCALL (XEXP (x, 0)))
>> +            assemble_external_libcall (XEXP (x, 0));
>> +        }
>
> This won't work if target can't use a direct call instruction.
> Consider
> __int128 a, b; void foo () { a = a / b; }
> on x86_64-linux.  With just -O2, the above works fine, with
> -O2 -mcmodel=large it will not, the call is indirect, but at least one has
> REG_CALL_DECL note that could be used as fallback to the above.
> And with -O0 -mcmodel=large because flag_ipa_ra is false REG_CALL_DECL isn't
> emitted at all.
> So, perhaps you could emit the REG_CALL_DECL note even if !flag_ipa_ra
> when SYMBOL_REF_LIBCALL is set?

Hmm something like this?

(I am aware that as things stand in emit_library_call_value_1 that
 conditional will be always true, but I think it is good to keep the
 conditional as documentation and in case emit_library_call_value_1
 changes in the future.  Note also that `fun' is known to be `orgfun'
 when the bit it set.  That may change later as per
 prepare_call_address.)

diff --git a/gcc/calls.cc b/gcc/calls.cc
index 1f3a6d5c450..219ea599b16 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -4388,9 +4388,10 @@ 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);
+  /* Mark the emitted target as a libcall.  This will be used by final
+     in order to emit an external symbol declaration if the libcall is
+     ever used.  */
+  SYMBOL_REF_LIBCALL (fun) = 1;
 
   original_args_size = args_size;
   args_size.constant = (aligned_upper_bound (args_size.constant
@@ -4735,7 +4736,7 @@ emit_library_call_value_1 (int retval, rtx orgfun, rtx 
value,
               valreg,
               old_inhibit_defer_pop + 1, call_fusage, flags, args_so_far);
 
-  if (flag_ipa_ra)
+  if (flag_ipa_ra || SYMBOL_REF_LIBCALL (orgfun))
     {
       rtx datum = orgfun;
       gcc_assert (GET_CODE (datum) == SYMBOL_REF);
diff --git a/gcc/final.cc b/gcc/final.cc
index dd3e22547ac..53f5d890809 100644
--- a/gcc/final.cc
+++ b/gcc/final.cc
@@ -804,6 +804,8 @@ make_pass_compute_alignments (gcc::context *ctxt)
 }
 
 
+static rtx call_from_call_insn (rtx_call_insn *insn);
+
 /* Make a pass over all insns and compute their actual lengths by shortening
    any branches of variable length if possible.  */
 
@@ -850,6 +852,19 @@ 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 or implementing a libcall,
+         and this machine requires an external definition for library
+         functions, write one out.  */
+      if (CALL_P (insn))
+        {
+          rtx x = call_from_call_insn (dyn_cast <rtx_call_insn *> (insn));
+
+          if ((x = XEXP (x, 0)) && MEM_P (x) && SYMBOL_REF_P (XEXP (x, 0))
+              || (x = find_reg_note (insn, REG_CALL_DECL, NULL_RTX)))
+            assemble_external_libcall (XEXP (x, 0));
+        }
+
       if (INSN_P (insn))
        continue;
 
>> diff --git a/gcc/rtl.h b/gcc/rtl.h
>> index e1c51156f90..945e3267a34 100644
>> --- a/gcc/rtl.h
>> +++ b/gcc/rtl.h
>> @@ -402,6 +402,8 @@ struct GTY((desc("0"), tag("0"),
>>       1 in a VALUE or DEBUG_EXPR is NO_LOC_P in var-tracking.cc.
>>       Dumped as "/i" in RTL dumps.  */
>>    unsigned return_val : 1;
>> +  /* 1 in a SYMBOL_REF if it is the target of a libcall.  */
>> +  unsigned is_libcall : 1;
>
> This is wrong.  struct rtx_def is carefully designed such that
> it has 16 + 8 + 8 bits before the union is 32-bit and then
> flexible array member which sometimes needs 64-bit alignment.
> So, the above change would grow all RTX by 64 bits.
> I think jump, call and in_struct are unused on SYMBOL_REF, so just
> document the new meaning of the chosen bit for SYMBOL_REF.

Ok good to know.  I will use `call' 8-)

Reply via email to