From: Christophe Lyon <christophe.lyon@linaro.org>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>,
Kyrylo Tkachov <kyrylo.tkachov@arm.com>,
gcc Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] arm: Fix up arm_override_options_after_change [PR96939]
Date: Fri, 11 Sep 2020 09:46:37 +0200 [thread overview]
Message-ID: <CAKdteObBT+nbZVFxx-YdAguCQks8+K6nEdYgM6R6jdE37NguVA@mail.gmail.com> (raw)
In-Reply-To: <20200908084512.GH18149@tucnak>
On Tue, 8 Sep 2020 at 10:45, Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Hi!
>
> As mentioned in the PR, the testcase fails to link, because when set_cfun is
> being called on the crc function, arm_override_options_after_change is
> called from set_cfun -> invoke_set_current_function_hook:
> /* Change optimization options if needed. */
> if (optimization_current_node != opts)
> {
> optimization_current_node = opts;
> cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts));
> }
> and at that point target_option_default_node actually matches even the
> current state of options, so this means armv7 (or whatever) arch is set as
> arm_active_target, then
> targetm.set_current_function (fndecl);
> is called later in that function, which because the crc function's
> DECL_FUNCTION_SPECIFIC_TARGET is different from the current one will do:
> cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
> which calls arm_option_restore and sets arm_active_target to armv8-a+crc
> (so far so good).
> Later arm_set_current_function calls:
> save_restore_target_globals (new_tree);
> which in this case calls:
> /* Call target_reinit and save the state for TARGET_GLOBALS. */
> TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
> which because optimization_current_node != optimization_default_node
> (the testcase is LTO, so all functions have their
> DECL_FUNCTION_SPECIFIC_TARGET and TREE_OPTIMIZATION nodes) will call:
> cl_optimization_restore
> (&global_options,
> TREE_OPTIMIZATION (optimization_default_node));
> and
> cl_optimization_restore (&global_options,
> TREE_OPTIMIZATION (opts));
> The problem is that these call arm_override_options_after_change again,
> and that one uses the target_option_default_node as what to set the
> arm_active_target to (i.e. back to armv7 or whatever, but not to the
> armv8-a+crc that should be the active target for the crc function).
> That means we then error on the builtin call in that function.
>
> Now, the targetm.override_options_after_change hook is called always at the
> end of cl_optimization_restore, i.e. when we change the Optimization marked
> generic options. So it seems unnecessary to call arm_configure_build_target
> at that point (nothing it depends on changed), and additionally incorrect
> (because it uses the target_option_default_node, rather than the current
> set of options; we'd need to revert
> https://gcc.gnu.org/legacy-ml/gcc-patches/2016-12/msg01390.html
> otherwise so that it works again with global_options otherwise).
> The options that arm_configure_build_target cares about will change only
> during option parsing (which is where it is called already), or during
> arm_set_current_function, where it is done during the
> cl_target_option_restore.
> Now, arm_override_options_after_change_1 wants to adjust the
> str_align_functions, which depends on the current Optimization options (e.g.
> optimize_size and flag_align_options and str_align_functions) as well as
> the target options target_flags, so IMHO needs to be called both
> when the Optimization options (possibly) change, i.e. from
> the targetm.override_options_after_change hook, and from when the target
> options change (set_current_function hook).
>
> Bootstrapped/regtested on armv7hl-linux-gnueabi, ok for trunk?
>
> Looking further at arm_override_options_after_change_1, it also seems to be
> incorrect, rather than testing
> !opts->x_str_align_functions
> it should be really testing
> !opts_set->x_str_align_functions
> and get &global_options_set or similar passed to it as additional opts_set
> argument. That is because otherwise the decision will be sticky, while it
> should be done whenever use provided -falign-functions but didn't provide
> -falign-functions= (either on the command line, or through optimize
> attribute or pragma).
>
> 2020-09-08 Jakub Jelinek <jakub@redhat.com>
>
> PR target/96939
> * config/arm/arm.c (arm_override_options_after_change): Don't call
> arm_configure_build_target here.
> (arm_set_current_function): Call arm_override_options_after_change_1
> at the end.
>
> * gcc.target/arm/lto/pr96939_0.c: New test.
> * gcc.target/arm/lto/pr96939_1.c: New file.
>
Hi Jakub,
I'm seeing an ICE with this new test on most of my arm configurations,
for instance:
--target arm-none-linux-gnueabi --with-cpu cortex-a9
/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-arm-none-linux-gnueabi/gcc3/gcc/xgcc
-B/aci-gcc-fsf/builds/gcc-fsf-gccsrc/obj-ar
m-none-linux-gnueabi/gcc3/gcc/ c_lto_pr96939_0.o c_lto_pr96939_1.o
-fdiagnostics-plain-output -flto -O2 -o
gcc-target-arm-lto-pr96939-01.exe
In function 'crc':
lto1: internal compiler error: Segmentation fault
0xba720f crash_signal
/gcc/toplev.c:327
0x172beb9 strchr
/usr/include/string.h:227
0x172beb9 arm_parse_cpu_option_name(cpu_option const*, char const*,
char const*, bool)
/gcc/common/config/arm/arm-common.c:349
0xfa9372 arm_configure_build_target(arm_build_target*,
cl_target_option*, gcc_options*, bool)
/gcc/config/arm/arm.c:3209
0xfbf8e1 arm_set_current_function
/gcc/config/arm/arm.c:32334
0x86b68b invoke_set_current_function_hook
/gcc/function.c:4670
0x870d77 invoke_set_current_function_hook
/gcc/function.c:4836
0x870d77 allocate_struct_function(tree_node*, bool)
/gcc/function.c:4793
0xa25943 input_function
/gcc/lto-streamer-in.c:1385
0xa25943 lto_read_body_or_constructor
/gcc/lto-streamer-in.c:1624
0x6f37ff cgraph_node::get_untransformed_body()
/gcc/cgraph.c:3932
0x703f42 cgraph_node::expand()
/gcc/cgraphunit.c:2274
0x70567c expand_all_functions
/gcc/cgraphunit.c:2476
0x70567c symbol_table::compile()
/gcc/cgraphunit.c:2839
0x63970b lto_main()
/gcc/lto/lto.c:653
This is with a cross-compiler.
Christophe
-
> --- gcc/config/arm/arm.c.jj 2020-07-30 15:04:38.136293101 +0200
> +++ gcc/config/arm/arm.c 2020-09-07 10:43:54.809561852 +0200
> @@ -3037,10 +3037,6 @@ arm_override_options_after_change_1 (str
> static void
> arm_override_options_after_change (void)
> {
> - arm_configure_build_target (&arm_active_target,
> - TREE_TARGET_OPTION (target_option_default_node),
> - &global_options_set, false);
> -
> arm_override_options_after_change_1 (&global_options);
> }
>
> @@ -32338,6 +32334,8 @@ arm_set_current_function (tree fndecl)
> cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
>
> save_restore_target_globals (new_tree);
> +
> + arm_override_options_after_change_1 (&global_options);
> }
>
> /* Implement TARGET_OPTION_PRINT. */
> --- gcc/testsuite/gcc.target/arm/lto/pr96939_0.c.jj 2020-09-07 11:26:45.909937609 +0200
> +++ gcc/testsuite/gcc.target/arm/lto/pr96939_0.c 2020-09-07 11:29:18.722706535 +0200
> @@ -0,0 +1,15 @@
> +/* PR target/96939 */
> +/* { dg-lto-do link } */
> +/* { dg-require-effective-target arm_arch_v8a_ok } */
> +/* { dg-lto-options { { -flto -O2 } } } */
> +
> +extern unsigned crc (unsigned, const void *);
> +typedef unsigned (*fnptr) (unsigned, const void *);
> +volatile fnptr fn;
> +
> +int
> +main ()
> +{
> + fn = crc;
> + return 0;
> +}
> --- gcc/testsuite/gcc.target/arm/lto/pr96939_1.c.jj 2020-09-07 11:26:49.365887153 +0200
> +++ gcc/testsuite/gcc.target/arm/lto/pr96939_1.c 2020-09-07 11:25:13.885281180 +0200
> @@ -0,0 +1,10 @@
> +/* PR target/96939 */
> +/* { dg-options "-march=armv8-a+crc" } */
> +
> +#include <arm_acle.h>
> +
> +unsigned
> +crc (unsigned x, const void *y)
> +{
> + return __crc32cw (x, *(unsigned *) y);
> +}
>
> Jakub
>
next prev parent reply other threads:[~2020-09-11 7:46 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-08 8:45 Jakub Jelinek
2020-09-08 22:01 ` Jeff Law
2020-09-10 8:51 ` [PATCH] arm: Fix up arm_override_options_after_change_1 Jakub Jelinek
2020-09-10 14:11 ` Kyrylo Tkachov
2020-09-10 14:58 ` Jeff Law
2020-09-10 15:01 ` Jakub Jelinek
2020-09-10 15:04 ` Kyrylo Tkachov
2020-09-11 7:46 ` Christophe Lyon [this message]
2020-09-11 9:29 ` [PATCH] arm: Fix up arm_override_options_after_change [PR96939] Jakub Jelinek
2020-09-13 8:29 ` [PATCH] options: Save and restore opts_set for Optimization and Target options Jakub Jelinek
2020-09-14 6:32 ` Richard Biener
2020-09-14 8:06 ` Christophe Lyon
2020-09-28 19:50 ` Stefan Schulze Frielinghaus
2020-09-28 19:58 ` Jakub Jelinek
2020-09-30 9:32 ` Jakub Jelinek
2020-09-30 11:21 ` Stefan Schulze Frielinghaus
2020-09-30 11:39 ` Jakub Jelinek
2020-09-30 13:24 ` Stefan Schulze Frielinghaus
2020-10-02 8:46 ` Jakub Jelinek
2020-10-02 14:21 ` Stefan Schulze Frielinghaus
2020-10-03 8:41 ` Jakub Jelinek
2020-10-03 18:02 ` Richard Biener
2020-10-04 7:13 ` Andreas Schwab
2020-10-04 19:16 ` Jakub Jelinek
2020-10-05 7:08 ` Jakub Jelinek
2020-10-05 7:10 ` Richard Biener
2020-10-06 9:28 ` Andreas Schwab
2020-10-06 13:20 ` [PATCH] options: Avoid unused variable mask warning [PR97305] Jakub Jelinek
2020-10-06 13:32 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAKdteObBT+nbZVFxx-YdAguCQks8+K6nEdYgM6R6jdE37NguVA@mail.gmail.com \
--to=christophe.lyon@linaro.org \
--cc=Richard.Earnshaw@foss.arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=kyrylo.tkachov@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).