On Tue, 2020-09-08 at 10:45 +0200, Jakub Jelinek via Gcc-patches 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 <ja...@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.
Any objection if I pull this into the Fedora tree and build a new GCC at some
point in the relatively new future (once approved). Similarly for your lto vs
linenumber patch?
Jeff
