On Mon, 17 Jun 2019, Tom de Vries wrote:
> Hi Alexander,
>
> any comments?
A couple suggestions (see below), but no serious concerns from my side.
> --- a/gcc/config/nvptx/nvptx.c
> +++ b/gcc/config/nvptx/nvptx.c
> @@ -112,6 +112,17 @@ enum nvptx_data_area
> DATA_AREA_MAX
> };
>
> +rtx
> +gen_set_softstack_insn (rtx op)
> +{
Here it might be appropriate to have something like
gcc_assert (GET_MODE (op) == Pmode);
because failure to supply a Pmode operand indicates a bug in the caller and
it may be desirable to catch it here; doesn't make sense to allow restoring
stack pointer from a 32-bit register with a 64-bit address space.
Likewise for other instances of this 'if (... DImode) ... else if (... SImode)
...'
pattern in the patch.
> + if (GET_MODE (op) == DImode)
> + return gen_set_softstack_insn_di (op);
> + else if (GET_MODE (op) == SImode)
> + return gen_set_softstack_insn_si (op);
> + else
> + gcc_unreachable ();
> +}
> +
> /* We record the data area in the target symbol flags. */
> #define SYMBOL_DATA_AREA(SYM) \
> (nvptx_data_area)((SYMBOL_REF_FLAGS (SYM) >> SYMBOL_FLAG_MACH_DEP_SHIFT) \
> diff --git a/gcc/config/nvptx/nvptx.md b/gcc/config/nvptx/nvptx.md
> index 3ed5296db96..1a676b942b2 100644
> --- a/gcc/config/nvptx/nvptx.md
> +++ b/gcc/config/nvptx/nvptx.md
> @@ -1071,8 +1071,8 @@
> DONE;
> })
>
> -(define_insn "set_softstack_insn"
> - [(unspec [(match_operand 0 "nvptx_register_operand" "R")]
> +(define_insn "set_softstack_insn_<mode>"
> + [(unspec [(match_operand:P 0 "nvptx_register_operand" "R")]
A purely cosmetic issue, but I'll mention it for completeness: the '_insn'
suffix is unnecessary with the '_<mode>' suffix added. This is how e.g. the
i386 backend deals with a similar issue, they have 'stack_protect_test' expand
and 'stack_protect_test_<mode>' insns.
Thanks!
Alexander
