diff mbox series

pragma: Update target option node when optimization changes [PR103515]

Message ID 6e9eaac5-3154-6460-1327-5c6afc14e33a@linux.ibm.com
State Committed
Commit 01ad8c54fdca1db3d71bf6c4b861a9d1db3c2a59
Headers
Series pragma: Update target option node when optimization changes [PR103515] |

Commit Message

Kewen.Lin Dec. 7, 2021, 2:15 a.m. UTC 
  Hi,

For a function with optimize pragma, it's possible that the target
options change as optimization options change.  Now we create one
optimization option node when parsing pragma optimize, but don't
create target option node for possible target option changes.  It
makes later processing not detect the target options have actually
changed and doesn't update the target options accordingly.

This patch is to check whether target options have changed when
creating one optimization option node for pragma optimize, and
make one target option node if needed.  The associated test case
shows the difference.  Without this patch, the function foo1 will
perform unrolling which is unexpected.  The reason is that flag
unroll_only_small_loops isn't correctly set for it.  The value
is updated after parsing function foo2, but doesn't get restored
later since both decls don't have DECL_FUNCTION_SPECIFIC_TARGET
set and the hook think we don't need to switch.  With this patch,
there is no unrolling for foo1, which is also consistent with the
behavior by replacing pragma by attribute whether w/ and w/o this
patch.

Bootstrapped and regtested on x86_64-redhat-linux, aarch64-linux-gnu
and powerpc64{,le}-linux-gnu.

Is it ok for trunk?

BR,
Kewen
---
gcc/ChangeLog:

	PR target/103515
	* attribs.c (decl_attributes): Check if target options change and
	create one node if so.

gcc/testsuite/ChangeLog:

	PR target/103515
	* gcc.target/powerpc/pr103515.c: New test.

-----
---
 gcc/attribs.c                               | 13 ++++++++-
 gcc/testsuite/gcc.target/powerpc/pr103515.c | 30 +++++++++++++++++++++
 2 files changed, 42 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr103515.c

Comments

Jeff Law Dec. 9, 2021, 1:43 a.m. UTC | #1 
On 12/6/2021 7:15 PM, Kewen.Lin via Gcc-patches wrote:
> Hi,
>
> For a function with optimize pragma, it's possible that the target
> options change as optimization options change.  Now we create one
> optimization option node when parsing pragma optimize, but don't
> create target option node for possible target option changes.  It
> makes later processing not detect the target options have actually
> changed and doesn't update the target options accordingly.
>
> This patch is to check whether target options have changed when
> creating one optimization option node for pragma optimize, and
> make one target option node if needed.  The associated test case
> shows the difference.  Without this patch, the function foo1 will
> perform unrolling which is unexpected.  The reason is that flag
> unroll_only_small_loops isn't correctly set for it.  The value
> is updated after parsing function foo2, but doesn't get restored
> later since both decls don't have DECL_FUNCTION_SPECIFIC_TARGET
> set and the hook think we don't need to switch.  With this patch,
> there is no unrolling for foo1, which is also consistent with the
> behavior by replacing pragma by attribute whether w/ and w/o this
> patch.
>
> Bootstrapped and regtested on x86_64-redhat-linux, aarch64-linux-gnu
> and powerpc64{,le}-linux-gnu.
>
> Is it ok for trunk?
>
> BR,
> Kewen
> ---
> gcc/ChangeLog:
>
> 	PR target/103515
> 	* attribs.c (decl_attributes): Check if target options change and
> 	create one node if so.
>
> gcc/testsuite/ChangeLog:
>
> 	PR target/103515
> 	* gcc.target/powerpc/pr103515.c: New test.
OK
jeff
Martin Liška Dec. 9, 2021, 3:21 p.m. UTC | #2 
On 12/7/21 03:15, Kewen.Lin wrote:
> Hi,
> 
> For a function with optimize pragma, it's possible that the target
> options change as optimization options change.  Now we create one
> optimization option node when parsing pragma optimize, but don't
> create target option node for possible target option changes.  It
> makes later processing not detect the target options have actually
> changed and doesn't update the target options accordingly.
> 
> This patch is to check whether target options have changed when
> creating one optimization option node for pragma optimize, and
> make one target option node if needed.  The associated test case
> shows the difference.  Without this patch, the function foo1 will
> perform unrolling which is unexpected.  The reason is that flag
> unroll_only_small_loops isn't correctly set for it.  The value
> is updated after parsing function foo2, but doesn't get restored
> later since both decls don't have DECL_FUNCTION_SPECIFIC_TARGET
> set and the hook think we don't need to switch.  With this patch,
> there is no unrolling for foo1, which is also consistent with the
> behavior by replacing pragma by attribute whether w/ and w/o this
> patch.
> 
> Bootstrapped and regtested on x86_64-redhat-linux, aarch64-linux-gnu
> and powerpc64{,le}-linux-gnu.
> 
> Is it ok for trunk?
> 
> BR,
> Kewen
> ---
> gcc/ChangeLog:
> 
> 	PR target/103515
> 	* attribs.c (decl_attributes): Check if target options change and
> 	create one node if so.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/103515
> 	* gcc.target/powerpc/pr103515.c: New test.
> 
> -----
> 

Hello.

I do support the patch as it does pretty similar thing to what I did in g:ebd5e86c0f41dc1d692f9b2b68a510b1f6835a3e.
The revision was about pragmas.

I can confirm the patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Martin
Kewen.Lin Dec. 13, 2021, 6:24 a.m. UTC | #3 
on 2021/12/9 上午9:43, Jeff Law wrote:
> 
> 
> On 12/6/2021 7:15 PM, Kewen.Lin via Gcc-patches wrote:
>> Hi,
>>
>> For a function with optimize pragma, it's possible that the target
>> options change as optimization options change.  Now we create one
>> optimization option node when parsing pragma optimize, but don't
>> create target option node for possible target option changes.  It
>> makes later processing not detect the target options have actually
>> changed and doesn't update the target options accordingly.
>>
>> This patch is to check whether target options have changed when
>> creating one optimization option node for pragma optimize, and
>> make one target option node if needed.  The associated test case
>> shows the difference.  Without this patch, the function foo1 will
>> perform unrolling which is unexpected.  The reason is that flag
>> unroll_only_small_loops isn't correctly set for it.  The value
>> is updated after parsing function foo2, but doesn't get restored
>> later since both decls don't have DECL_FUNCTION_SPECIFIC_TARGET
>> set and the hook think we don't need to switch.  With this patch,
>> there is no unrolling for foo1, which is also consistent with the
>> behavior by replacing pragma by attribute whether w/ and w/o this
>> patch.
>>
>> Bootstrapped and regtested on x86_64-redhat-linux, aarch64-linux-gnu
>> and powerpc64{,le}-linux-gnu.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> ---
>> gcc/ChangeLog:
>>
>>     PR target/103515
>>     * attribs.c (decl_attributes): Check if target options change and
>>     create one node if so.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     PR target/103515
>>     * gcc.target/powerpc/pr103515.c: New test.
> OK
> jeff
> 

Thanks for the review, Jeff!  Committed as r12-5920.

BR,
Kewen
Kewen.Lin Dec. 13, 2021, 6:40 a.m. UTC | #4 
on 2021/12/9 下午11:21, Martin Liška wrote:
> On 12/7/21 03:15, Kewen.Lin wrote:
>> Hi,
>>
>> For a function with optimize pragma, it's possible that the target
>> options change as optimization options change.  Now we create one
>> optimization option node when parsing pragma optimize, but don't
>> create target option node for possible target option changes.  It
>> makes later processing not detect the target options have actually
>> changed and doesn't update the target options accordingly.
>>
>> This patch is to check whether target options have changed when
>> creating one optimization option node for pragma optimize, and
>> make one target option node if needed.  The associated test case
>> shows the difference.  Without this patch, the function foo1 will
>> perform unrolling which is unexpected.  The reason is that flag
>> unroll_only_small_loops isn't correctly set for it.  The value
>> is updated after parsing function foo2, but doesn't get restored
>> later since both decls don't have DECL_FUNCTION_SPECIFIC_TARGET
>> set and the hook think we don't need to switch.  With this patch,
>> there is no unrolling for foo1, which is also consistent with the
>> behavior by replacing pragma by attribute whether w/ and w/o this
>> patch.
>>
>> Bootstrapped and regtested on x86_64-redhat-linux, aarch64-linux-gnu
>> and powerpc64{,le}-linux-gnu.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> ---
>> gcc/ChangeLog:
>>
>>     PR target/103515
>>     * attribs.c (decl_attributes): Check if target options change and
>>     create one node if so.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     PR target/103515
>>     * gcc.target/powerpc/pr103515.c: New test.
>>
>> -----
>>
> 
> Hello.
> 
> I do support the patch as it does pretty similar thing to what I did in g:ebd5e86c0f41dc1d692f9b2b68a510b1f6835a3e.
> The revision was about pragmas.
> 
> I can confirm the patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Martin

Hi Martin,

Thanks for the feedback and the pointer to your previous commit for the similar issue,
I've added it in the commit log as one reference.

BR,
Kewen
diff mbox series

Patch

diff --git a/gcc/attribs.c b/gcc/attribs.c
index c252f5af07b..761d9760769 100644
--- a/gcc/attribs.c
+++ b/gcc/attribs.c
@@ -607,7 +607,18 @@  decl_attributes (tree *node, tree attributes, int flags,
   if (TREE_CODE (*node) == FUNCTION_DECL
       && optimization_current_node != optimization_default_node
       && !DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node))
-    DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = optimization_current_node;
+    {
+      DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = optimization_current_node;
+      tree cur_tree
+	= build_target_option_node (&global_options, &global_options_set);
+      tree old_tree = DECL_FUNCTION_SPECIFIC_TARGET (*node);
+      if (!old_tree)
+	old_tree = target_option_default_node;
+      /* The changes on optimization options can cause the changes in
+	 target options, update it accordingly if it's changed.  */
+      if (old_tree != cur_tree)
+	DECL_FUNCTION_SPECIFIC_TARGET (*node) = cur_tree;
+    }
 
   /* If this is a function and the user used #pragma GCC target, add the
      options to the attribute((target(...))) list.  */
diff --git a/gcc/testsuite/gcc.target/powerpc/pr103515.c b/gcc/testsuite/gcc.target/powerpc/pr103515.c
new file mode 100644
index 00000000000..698b9a93037
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr103515.c
@@ -0,0 +1,30 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll-optimized" } */
+
+/* The pragma specified for foo2 should not affect foo1.
+   Verify compiler won't perform unrolling for foo1.  */
+
+#define N 1024
+extern int a1[N], b1[N], c1[N];
+extern int a2[N], b2[N], c2[N];
+extern int n;
+
+void
+foo1 ()
+{
+  int i;
+  for (i = 0; i < n; i++)
+    c1[i] += a1[i] + b1[i];
+}
+
+#pragma GCC optimize("O3,unroll-loops")
+void
+foo2 ()
+{
+  int i;
+  for (i = 0; i < n; i++)
+    c2[i] += a2[i] + b2[i];
+}
+
+/* { dg-final { scan-rtl-dump-times "optimized: loop unrolled" 1 "loop2_unroll" } } */
+