On Fri, Apr 16, 2021 at 12:02 PM Richard Sandiford via Gcc-patches
<[email protected]> wrote:
>
> This patch fixes a regression introduced by the rtl-ssa patches.
> It was seen on HPPA but it might be latent elsewhere.
>
> The problem is that the traditional way of expanding an untyped_call
> is to emit sequences like:
>
> (call (mem (symbol_ref "foo")))
> (set (reg pseudo1) (reg result1))
> ...
> (set (reg pseudon) (reg resultn))
>
> The ABI specifies that result1..resultn are clobbered by the call but
> nothing in the RTL indicates that result1..resultn are the results
> of the call. Normally, using a clobbered value gives undefined results,
> but in this case the results are well-defined and matter for correctness.
>
> This seems like a niche case, so I think it would be better to mark
> it explicitly rather than try to detect it heuristically.
>
> Note that in expand_builtin_apply we already have an rtx_insn *,
> so it doesn't matter whether we call emit_call_insn or emit_insn.
> Calling emit_insn seems more natural now that the gen_* call
> has been split out. It also matches later code in the function.
>
> Tested on aarch64-linux-gnu, arm-linux-gnueabihf, armeb-eabi and
> x86_64-linux-gnu. OK to install?
OK. Does DF handle this correctly?
I wonder why we cannot simply do sth like
(call_insn (set (parallel [ (reg:...) (reg:...) ] ) (mem (symbol_ref...)))
or so? But I guess we'd have to alter all targets for this...
Thanks,
Richard.
> Richard
>
>
> gcc/
> PR rtl-optimization/98689
> * reg-notes.def (UNTYPED_CALL): New note.
> * combine.c (distribute_notes): Handle it.
> * emit-rtl.c (try_split): Likewise.
> * rtlanal.c (rtx_properties::try_to_add_insn): Likewise. Assume
> that calls with the note implicitly set all return value registers.
> * builtins.c (expand_builtin_apply): Add a REG_UNTYPED_CALL
> to untyped_calls.
> ---
> gcc/builtins.c | 8 ++++++--
> gcc/combine.c | 1 +
> gcc/emit-rtl.c | 1 +
> gcc/reg-notes.def | 15 +++++++++++++++
> gcc/rtlanal.c | 9 +++++++++
> 5 files changed, 32 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index 196dda3fa5e..d30c4eb62fc 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -2490,8 +2490,12 @@ expand_builtin_apply (rtx function, rtx arguments, rtx
> argsize)
> if (targetm.have_untyped_call ())
> {
> rtx mem = gen_rtx_MEM (FUNCTION_MODE, function);
> - emit_call_insn (targetm.gen_untyped_call (mem, result,
> - result_vector (1, result)));
> + rtx_insn *seq = targetm.gen_untyped_call (mem, result,
> + result_vector (1, result));
> + for (rtx_insn *insn = seq; insn; insn = NEXT_INSN (insn))
> + if (CALL_P (insn))
> + add_reg_note (insn, REG_UNTYPED_CALL, NULL_RTX);
> + emit_insn (seq);
> }
> else if (targetm.have_call_value ())
> {
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 6cb5b0ca102..9063a07bd00 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -14337,6 +14337,7 @@ distribute_notes (rtx notes, rtx_insn *from_insn,
> rtx_insn *i3, rtx_insn *i2,
> case REG_SETJMP:
> case REG_TM:
> case REG_CALL_DECL:
> + case REG_UNTYPED_CALL:
> case REG_CALL_NOCF_CHECK:
> /* These notes must remain with the call. It should not be
> possible for both I2 and I3 to be a call. */
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 1113c58c49e..07e908624a0 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -3964,6 +3964,7 @@ try_split (rtx pat, rtx_insn *trial, int last)
> break;
>
> case REG_CALL_DECL:
> + case REG_UNTYPED_CALL:
> gcc_assert (call_insn != NULL_RTX);
> add_reg_note (call_insn, REG_NOTE_KIND (note), XEXP (note, 0));
> break;
> diff --git a/gcc/reg-notes.def b/gcc/reg-notes.def
> index d7f0bfdab0b..995052ebc28 100644
> --- a/gcc/reg-notes.def
> +++ b/gcc/reg-notes.def
> @@ -233,6 +233,21 @@ REG_NOTE (STACK_CHECK)
> insn. This note is a SYMBOL_REF. */
> REG_NOTE (CALL_DECL)
>
> +/* Indicates that the call is an untyped_call. These calls are special
> + in that they set all of the target ABI's return value registers to a
> + defined value without explicitly saying so. For example, a typical
> + untyped_call sequence has the form:
> +
> + (call (mem (symbol_ref "foo")))
> + (set (reg pseudo1) (reg result1))
> + ...
> + (set (reg pseudon) (reg resultn))
> +
> + The ABI specifies that result1..resultn are clobbered by the call,
> + but the RTL does not indicate that result1..resultn are the results
> + of the call. */
> +REG_NOTE (UNTYPED_CALL)
> +
> /* Indicate that a call should not be verified for control-flow consistency.
> The target address of the call is assumed as a valid address and no check
> to validate a branch to the target address is needed. The call is marked
> diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
> index 170420a610b..67a49e65fd8 100644
> --- a/gcc/rtlanal.c
> +++ b/gcc/rtlanal.c
> @@ -2333,6 +2333,15 @@ rtx_properties::try_to_add_insn (const rtx_insn *insn,
> bool include_notes)
> *ref_iter++ = rtx_obj_reference (regno, flags,
> reg_raw_mode[regno], 0);
> }
> + /* Untyped calls implicitly set all function value registers.
> + Again, we add them first in case the main pattern contains
> + a fixed-form clobber. */
> + if (find_reg_note (insn, REG_UNTYPED_CALL, NULL_RTX))
> + for (unsigned int regno = 0; regno < FIRST_PSEUDO_REGISTER; ++regno)
> + if (targetm.calls.function_value_regno_p (regno)
> + && ref_iter != ref_end)
> + *ref_iter++ = rtx_obj_reference (regno, rtx_obj_flags::IS_WRITE,
> + reg_raw_mode[regno], 0);
> if (ref_iter != ref_end && !RTL_CONST_CALL_P (insn))
> {
> auto mem_flags = rtx_obj_flags::IS_READ;
