On Wed, 20 Nov 2019, Matthew Malcomson wrote:
> When compiling __RTL functions with a start pass, `skip_pass` needs to
> set global state when skipping a pass that would have marked something
> for future passes to see.
>
> Existing examples are setting `reload_completed` and
> `epilogue_completed` when skipping the reload and pro_and_epilogue
> passes respectively.
>
> This general problem occurs again in the AArch64 backend, where the
> cfun->machine->frame.laid_out variable is set on each function when
> `aarch64_layout_frame` is called (which can happen in both ira and
> reload.
> Currently any `return` statement will trigger an assert in the AArch64
> backend due to checking if address signing is enabled when the above
> variable is not set.
>
> To account for the need to have a backend specific settings we add a
> target hook that each backend can implement as required for their own
> global state.
>
> Here we also account for the motivating example by implementing this
> hook for the AArch64 backend.
Hmm, so this is only sensible when the RTL IL contains all state
besides the flag stating the state is present. Which makes me
wonder how the aarch64 backend actually needs this flag and
why it is not derivable from existing global state
(reload_completed || lra_in_progress maybe?).
That is, this hook shouldn't be necessary, and if it is then
backends might be tempted to do actual work on the IL as if
the IL was in the state of the pass being skipped - which can be
not the case at all if the IL is for a much later pass.
So -- not OK without further explanation.
Thanks,
Richard.
> regtested on x86_64 and aarch64.
>
> gcc/ChangeLog:
>
> 2019-11-20 Matthew Malcomson <matthew.malcom...@arm.com>
>
> * config/aarch64/aarch64.c (aarch64_skip_pass): New.
> (TARGET_BACKEND_SKIP_PASS): New.
> * doc/tm.texi: Document BACKEND_SKIP_PASS hook.
> * doc/tm.texi.in: Document BACKEND_SKIP_PASS hook.
> * passes.c (skip_pass): Call targetm.backend_skip_pass if it's
> defined.
> * target.def (backend_skip_pass): Introduce new hook.
>
> gcc/testsuite/ChangeLog:
>
> 2019-11-20 Matthew Malcomson <matthew.malcom...@arm.com>
>
> * gcc.dg/rtl/aarch64/return-statement.c: New test.
>
>
>
> ############### Attachment also inlined for ease of reply
> ###############
>
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index
> 599d07a729e7438080f8b5240ee95037a49fb983..fbe900dec8d5ef3daf7be691cdaf4350c13d5024
> 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -75,6 +75,7 @@
> #include "intl.h"
> #include "expmed.h"
> #include "function-abi.h"
> +#include "tree-pass.h"
>
> /* This file should be included last. */
> #include "target-def.h"
> @@ -21371,6 +21372,14 @@ aarch64_stack_protect_guard (void)
> return NULL_TREE;
> }
>
> +/* Implement TARGET_BACKEND_SKIP_PASS. We need to set
> + cfun->machine->frame.laid_out when skipping the ira pass. */
> +void aarch64_skip_pass (opt_pass *pass)
> +{
> + if (strcmp (pass->name, "ira") == 0)
> + cfun->machine->frame.laid_out = true;
> +}
> +
> /* Implement TARGET_ASM_FILE_END for AArch64. This adds the AArch64 GNU NOTE
> section at the end if needed. */
> #define GNU_PROPERTY_AARCH64_FEATURE_1_AND 0xc0000000
> @@ -21947,6 +21956,9 @@ aarch64_libgcc_floating_mode_supported_p
> #undef TARGET_STRICT_ARGUMENT_NAMING
> #define TARGET_STRICT_ARGUMENT_NAMING hook_bool_CUMULATIVE_ARGS_true
>
> +#undef TARGET_BACKEND_SKIP_PASS
> +#define TARGET_BACKEND_SKIP_PASS aarch64_skip_pass
> +
> struct gcc_target targetm = TARGET_INITIALIZER;
>
> #include "gt-aarch64.h"
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index
> cd9aed9874f4e6b2b0e2f8956ed6155975e643a8..d02fd747b479246cfdf7c7e6af03a1a2bd43e1c4
> 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -12152,3 +12152,7 @@ This target hook can be used to generate a
> target-specific code
> @deftypefn {Target Hook} void TARGET_RUN_TARGET_SELFTESTS (void)
> If selftests are enabled, run any selftests for this target.
> @end deftypefn
> +
> +@deftypefn {Target Hook} void TARGET_BACKEND_SKIP_PASS (opt_pass *@var{pass})
> +Given pass currently skipping, run any actions to set up state. When
> compiling code from the RTL frontend, some passes are skipped. If a given
> pass would set backend state to mark that a given action has been done, then
> the backend should implement this hook to set the same state when that pass
> is skipped. As an example, the AArch64 backend sets the backend specific
> cfun->machine->frame.laid_out structure member when the ira pass is skipped.
> +@end deftypefn
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index
> 2739e9ceec5ad7253ff9135da8dbe3bf6010e8d7..7cf446a88abe1eb3bfdcf1c685fc19905d46fca0
> 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -8194,3 +8194,5 @@ maintainer is familiar with.
> @hook TARGET_SPECULATION_SAFE_VALUE
>
> @hook TARGET_RUN_TARGET_SELFTESTS
> +
> +@hook TARGET_BACKEND_SKIP_PASS
> diff --git a/gcc/passes.c b/gcc/passes.c
> index
> d86af115ecb16fcab6bfce070f1f3e4f1d90ce71..16b3eb106f52da88d4a5717cdc4b66efd4f2c9d4
> 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -2416,6 +2416,9 @@ skip_pass (opt_pass *pass)
> rtl_register_cfg_hooks ();
> cfun->curr_properties &= ~PROP_cfglayout;
> }
> +
> + if (targetm.backend_skip_pass)
> + targetm.backend_skip_pass(pass);
> }
>
> /* Execute PASS. */
> diff --git a/gcc/target.def b/gcc/target.def
> index
> 8e83c2c7a7136511c07a5bc9e18876c91a38b955..f5313fa1ebe50875559e68792991c6ea37e15bcb
> 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -6765,6 +6765,12 @@ DEFHOOK
> void, (void),
> NULL)
>
> +DEFHOOK
> +(backend_skip_pass,
> + "Given pass currently skipping, run any actions to set up state. When
> compiling code from the RTL frontend, some passes are skipped. If a given
> pass would set backend state to mark that a given action has been done, then
> the backend should implement this hook to set the same state when that pass
> is skipped. As an example, the AArch64 backend sets the backend specific
> cfun->machine->frame.laid_out structure member when the ira pass is skipped.",
> + void, (opt_pass *pass), NULL)
> +
> +
> /* Close the 'struct gcc_target' definition. */
> HOOK_VECTOR_END (C90_EMPTY_HACK)
>
> diff --git a/gcc/testsuite/gcc.dg/rtl/aarch64/return-statement.c
> b/gcc/testsuite/gcc.dg/rtl/aarch64/return-statement.c
> new file mode 100644
> index
> 0000000000000000000000000000000000000000..4fae39b4c3d657063f07f6c4a23e2eb7e1d46068
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/rtl/aarch64/return-statement.c
> @@ -0,0 +1,41 @@
> +/* { dg-do compile { target aarch64-*-* } } */
> +
> +/* Testing that the aarch64 `backend_skip_pass` implementation sets the
> + relevant value so that `return` does not cause an assertion failure later
> + on. */
> +int __RTL (startwith ("cprop_hardreg")) foo2 (int a)
> +{
> +(function "foo2"
> + (param "a"
> + (DECL_RTL (reg/v:SI <1> [ a ]))
> + (DECL_RTL_INCOMING (reg:SI x0 [ a ])))
> + (insn-chain
> + (cnote 1 NOTE_INSN_DELETED)
> + (block 2
> + (edge-from entry (flags "FALLTHRU"))
> + (cnote 4 [bb 2] NOTE_INSN_BASIC_BLOCK)
> + (cnote 17 NOTE_INSN_PROLOGUE_END)
> + (cnote 2 NOTE_INSN_DELETED)
> + (cnote 3 NOTE_INSN_FUNCTION_BEG)
> + (cnote 6 NOTE_INSN_DELETED)
> + (cinsn:TI 11 (set (reg/i:SI x0)
> + (plus:SI (reg:SI x0 [95])
> + (const_int 1))) "some-test-file.c":7)
> + (cinsn 12 (use (reg/i:SI x0)) "some-test-file.c":7)
> + (cnote 24 NOTE_INSN_EPILOGUE_BEG)
> + (cinsn 19 (use (reg:DI x30)) "some-test-file.c":7)
> + (cjump_insn 20 (return) "some-test-file.c":7)
> + (edge-to exit)
> + ) ;; block 2
> + (cbarrier 21)
> + (cnote 15 NOTE_INSN_DELETED)
> + (cnote 16 NOTE_INSN_DELETED)
> + ) ;; insn-chain
> + (crtl
> + (return_rtx
> + (reg/i:SI x0)
> + ) ;; return_rtx
> + ) ;; crtl
> +) ;; function "foo2"
> +}
> +
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
