public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] arm: Fix up arm_override_options_after_change [PR96939]
@ 2020-09-08  8:45 Jakub Jelinek
  2020-09-08 22:01 ` Jeff Law
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Jakub Jelinek @ 2020-09-08  8:45 UTC (permalink / raw)
  To: Richard Earnshaw, Kyrylo Tkachov; +Cc: gcc-patches

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.

--- 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


^ permalink raw reply	[flat|nested] 29+ messages in thread
end of thread, other threads:[~2020-10-06 13:32 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08  8:45 [PATCH] arm: Fix up arm_override_options_after_change [PR96939] 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 ` [PATCH] arm: Fix up arm_override_options_after_change [PR96939] Christophe Lyon
2020-09-11  9:29   ` 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

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).