[PATCH] Fix target clone indirection elimination.
Richard Biener
richard.guenther@gmail.com
Mon Jun 22 09:24:16 GMT 2020
On Sun, Jun 21, 2020 at 2:32 AM Yichao Yu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Sat, Jun 20, 2020 at 8:16 PM Yichao Yu <yyc1992@gmail.com> wrote:
> >
> > On Sat, Jun 20, 2020 at 3:41 PM Yichao Yu <yyc1992@gmail.com> wrote:
> > >
> > > On Sat, Jun 20, 2020 at 3:26 PM Yichao Yu <yyc1992@gmail.com> wrote:
> > > >
> > > > On Sat, Jun 20, 2020 at 3:25 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > >
> > > > > On Sat, Jun 20, 2020 at 12:20 PM Yichao Yu <yyc1992@gmail.com> wrote:
> > > > > >
> > > > > > On Sat, Jun 20, 2020 at 3:12 PM H.J. Lu <hjl.tools@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sat, Jun 20, 2020 at 11:37 AM Yichao Yu <yyc1992@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Jun 20, 2020 at 1:54 PM Yichao Yu <yyc1992@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > > > The current logic seems to be comparing the whole attribute tree between the callee
> > > > > > > > > > > and caller (or at least the tree starting from the target attribute).
> > > > > > > > > > > This is unnecessary and causes strange dependency of the indirection
> > > > > > > > > > > elimination on unrelated properties like `noinline`(PR95780) and `visibility`(PR95778).
> > > > > > > > > >
> > > > > > > > > > Does it fix PR95780 and PR95778? Can you include testcases for them?
> > > > > > > >
> > > > > > > > OK so replacing
> > > > > > > > https://github.com/gcc-mirror/gcc/commit/b8ce8129a560f64f8b2855c4a3812b7c3c0ebf3f#diff-e2d535917af8555baad2e9c8749e96a5
> > > > > > > > with/adding to the test the following one should work. I still
> > > > > > > > couldn't get test to run though......
> > > > > > > >
> > > > > > > Can you try the enclosed testcases?
> > > > > >
> > > > > >
> > > > > > The assembly produced are the following with my patch and if I
> > > > > > understand it correctly those should work. Unfortunately I don't know
> > > > > > how to actually run the test as a test (if that makes sense....).
> > > > > >
> > > > >
> > > > > Place 2 files under gcc/testsuite/gcc.target/i386 and in GCC build directory, do
> > > > >
> > > > > $ make check-gcc RUNTESTFLAGS="--target_board='unix{-m32,}'
> > > > > i386.exp=pr95778-*.c"
> > > > >
> > >
> > > Finally got it start running after installing dejagnu...
> > > It seems to be runing something unrelated though....
> > >
> > > e.g. Running /home/yuyichao/projects/contrib/gcc/gcc/testsuite/gcc.c-torture/execute/builtins/builtins.exp
> > > ...
> > > Running /home/yuyichao/projects/contrib/gcc/gcc/testsuite/gcc.c-torture/execute/execute.exp
> >
> > OK it seems that the `-*.c` syntax doesn't work for me somehow, there
> > are still a lot of unrelated outputs but when I replaced the * with 1
> > and 2 separately I can confirm that there are only expected pass with
> > my patch and there are some unexpected failures when the patch is
> > reverted.
>
> And the updated patch is
OK.
Thanks,
Richard,
> From b219facb0960cd09bbcef72e9c03cc045474a8e6 Mon Sep 17 00:00:00 2001
> From: Yichao Yu <yyc1992@gmail.com>
> Date: Sat, 20 Jun 2020 11:59:36 -0400
> Subject: [PATCH] Fix target clone indirection elimination.
>
> The current logic seems to be comparing the whole attribute tree
> between the callee
> and caller (or at least the tree starting from the target attribute).
> This is unnecessary and causes strange dependency of the indirection
> elimination on unrelated properties like `noinline`(PR95780) and
> `visibility`(PR95778).
>
> This changes the comparison to be only on the `target` attribute which should be
> the intent of the code.
> ---
> gcc/multiple_target.c | 4 ++--
> gcc/testsuite/gcc.target/i386/pr95778-1.c | 21 +++++++++++++++++++++
> gcc/testsuite/gcc.target/i386/pr95778-2.c | 21 +++++++++++++++++++++
> 3 files changed, 44 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/i386/pr95778-1.c
> create mode 100644 gcc/testsuite/gcc.target/i386/pr95778-2.c
>
> diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
> index c1cfe8ff978..9eb0afd58cc 100644
> --- a/gcc/multiple_target.c
> +++ b/gcc/multiple_target.c
> @@ -483,7 +483,7 @@ redirect_to_specific_clone (cgraph_node *node)
> DECL_ATTRIBUTES (e->callee->decl));
>
> /* Function is not calling proper target clone. */
> - if (!attribute_list_equal (attr_target, attr_target2))
> + if (attr_target2 == NULL_TREE || !attribute_value_equal
> (attr_target, attr_target2))
> {
> while (fv2->prev != NULL)
> fv2 = fv2->prev;
> @@ -494,7 +494,7 @@ redirect_to_specific_clone (cgraph_node *node)
> cgraph_node *callee = fv2->this_node;
> attr_target2 = lookup_attribute ("target",
> DECL_ATTRIBUTES (callee->decl));
> - if (attribute_list_equal (attr_target, attr_target2))
> + if (attr_target2 != NULL_TREE && attribute_value_equal
> (attr_target, attr_target2
> ))
> {
> e->redirect_callee (callee);
> cgraph_edge::redirect_call_stmt_to_callee (e);
> diff --git a/gcc/testsuite/gcc.target/i386/pr95778-1.c
> b/gcc/testsuite/gcc.target/i386/pr95778-
> 1.c
> new file mode 100644
> index 00000000000..3238303d696
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr95778-1.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O3 -fPIC -fno-asynchronous-unwind-tables" } */
> +/* { dg-require-ifunc "" } */
> +
> +__attribute__((target_clones("default,avx2")))
> +static int
> +f2(int *p)
> +{
> + asm volatile ("" :: "r"(p) : "memory");
> + return *p;
> +}
> +
> +__attribute__((target_clones("default,avx2")))
> +int
> +g2(int *p)
> +{
> + return f2(p);
> +}
> +
> +/* { dg-final { scan-assembler "g2.default.1:\n\tjmp\tf2.default.1\n" } } */
> +/* { dg-final { scan-assembler "g2.avx2.0:\n\tjmp\tf2.avx2.0\n" } } */
> diff --git a/gcc/testsuite/gcc.target/i386/pr95778-2.c
> b/gcc/testsuite/gcc.target/i386/pr95778-
> 2.c
> new file mode 100644
> index 00000000000..e88702d2b82
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr95778-2.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile { target fpic } } */
> +/* { dg-options "-O3 -fPIC -fno-asynchronous-unwind-tables" } */
> +/* { dg-require-ifunc "" } */
> +
> +__attribute__((visibility("internal"),target_clones("default,avx2")))
> +int
> +f2(int *p)
> +{
> + asm volatile ("" :: "r"(p) : "memory");
> + return *p;
> +}
> +
> +__attribute__((target_clones("default,avx2")))
> +int
> +g2(int *p)
> +{
> + return f2(p);
> +}
> +
> +/* { dg-final { scan-assembler "g2.default.1:\n\tjmp\tf2.default.1\n" } } */
> +/* { dg-final { scan-assembler "g2.avx2.0:\n\tjmp\tf2.avx2.0\n" } } */
> --
> 2.27.0
More information about the Gcc-patches
mailing list