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-)