diff mbox series

Check optab before transforming atomic bit test and operations

Message ID 20211115133943.440104-1-hjl.tools@gmail.com
State New
Headers
Series Check optab before transforming atomic bit test and operations |

Commit Message

H.J. Lu Nov. 15, 2021, 1:39 p.m. UTC 
  Check optab before transforming equivalent, but slighly different cases
of atomic bit test and operations to their canonical forms.

gcc/

	PR middle-end/103184
	* tree-ssa-ccp.c (optimize_atomic_bit_test_and): Check optab
	before transforming equivalent, but slighly different cases to
	their canonical forms.

gcc/testsuite/

	PR middle-end/103184
	* gcc.dg/pr103184-1.c: New test.
	* gcc.dg/pr103184-2.c: Likewise.
---
 gcc/testsuite/gcc.dg/pr103184-1.c | 43 +++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.dg/pr103184-2.c | 12 +++++++++
 gcc/tree-ssa-ccp.c                | 34 +++++++++++++-----------
 3 files changed, 74 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/pr103184-1.c
 create mode 100644 gcc/testsuite/gcc.dg/pr103184-2.c

Comments

Jeff Law Nov. 15, 2021, 6:59 p.m. UTC | #1 
On 11/15/2021 6:39 AM, H.J. Lu via Gcc-patches wrote:
> Check optab before transforming equivalent, but slighly different cases
> of atomic bit test and operations to their canonical forms.
>
> gcc/
>
> 	PR middle-end/103184
> 	* tree-ssa-ccp.c (optimize_atomic_bit_test_and): Check optab
> 	before transforming equivalent, but slighly different cases to
> 	their canonical forms.
>
> gcc/testsuite/
>
> 	PR middle-end/103184
> 	* gcc.dg/pr103184-1.c: New test.
> 	* gcc.dg/pr103184-2.c: Likewise.

>   	}
>       }
>   
> -  switch (fn)
> -    {
> -    case IFN_ATOMIC_BIT_TEST_AND_SET:
> -      optab = atomic_bit_test_and_set_optab;
> -      break;
> -    case IFN_ATOMIC_BIT_TEST_AND_COMPLEMENT:
> -      optab = atomic_bit_test_and_complement_optab;
> -      break;
> -    case IFN_ATOMIC_BIT_TEST_AND_RESET:
> -      optab = atomic_bit_test_and_reset_optab;
> -      break;
> -    default:
> -      return;
> -    }
> -
>     if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs))) == CODE_FOR_nothing)
>       return;
Shouldn't the test of the return value of optab_handler here just go 
away since we're testing it earlier?  OK with that fix.

Jeff
H.J. Lu Nov. 15, 2021, 7:05 p.m. UTC | #2 
On Mon, Nov 15, 2021 at 10:59 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 11/15/2021 6:39 AM, H.J. Lu via Gcc-patches wrote:
> > Check optab before transforming equivalent, but slighly different cases
> > of atomic bit test and operations to their canonical forms.
> >
> > gcc/
> >
> >       PR middle-end/103184
> >       * tree-ssa-ccp.c (optimize_atomic_bit_test_and): Check optab
> >       before transforming equivalent, but slighly different cases to
> >       their canonical forms.
> >
> > gcc/testsuite/
> >
> >       PR middle-end/103184
> >       * gcc.dg/pr103184-1.c: New test.
> >       * gcc.dg/pr103184-2.c: Likewise.
>
> >       }
> >       }
> >
> > -  switch (fn)
> > -    {
> > -    case IFN_ATOMIC_BIT_TEST_AND_SET:
> > -      optab = atomic_bit_test_and_set_optab;
> > -      break;
> > -    case IFN_ATOMIC_BIT_TEST_AND_COMPLEMENT:
> > -      optab = atomic_bit_test_and_complement_optab;
> > -      break;
> > -    case IFN_ATOMIC_BIT_TEST_AND_RESET:
> > -      optab = atomic_bit_test_and_reset_optab;
> > -      break;
> > -    default:
> > -      return;
> > -    }
> > -
> >     if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs))) == CODE_FOR_nothing)
> >       return;
> Shouldn't the test of the return value of optab_handler here just go
> away since we're testing it earlier?  OK with that fix.
>

The earlier check is predicated on if (rhs_code != BIT_AND_EXPR):

  if (rhs_code != BIT_AND_EXPR)
    {
      if (rhs_code != NOP_EXPR && rhs_code != BIT_NOT_EXPR)
        return;

      tree use_lhs = gimple_assign_lhs (use_stmt);
      if (TREE_CODE (use_lhs) == SSA_NAME
          && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (use_lhs))
        return;

      tree use_rhs = gimple_assign_rhs1 (use_stmt);
      if (lhs != use_rhs)
        return;

      if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs)))
          == CODE_FOR_nothing)
        return;

I can add an "else"

else  if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs)))
            == CODE_FOR_nothing)
    return;

Will it be OK?

Thanks.
Jeff Law Nov. 15, 2021, 7:14 p.m. UTC | #3 
On 11/15/2021 12:05 PM, H.J. Lu wrote:
> On Mon, Nov 15, 2021 at 10:59 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>>
>>
>> On 11/15/2021 6:39 AM, H.J. Lu via Gcc-patches wrote:
>>> Check optab before transforming equivalent, but slighly different cases
>>> of atomic bit test and operations to their canonical forms.
>>>
>>> gcc/
>>>
>>>        PR middle-end/103184
>>>        * tree-ssa-ccp.c (optimize_atomic_bit_test_and): Check optab
>>>        before transforming equivalent, but slighly different cases to
>>>        their canonical forms.
>>>
>>> gcc/testsuite/
>>>
>>>        PR middle-end/103184
>>>        * gcc.dg/pr103184-1.c: New test.
>>>        * gcc.dg/pr103184-2.c: Likewise.
>>>        }
>>>        }
>>>
>>> -  switch (fn)
>>> -    {
>>> -    case IFN_ATOMIC_BIT_TEST_AND_SET:
>>> -      optab = atomic_bit_test_and_set_optab;
>>> -      break;
>>> -    case IFN_ATOMIC_BIT_TEST_AND_COMPLEMENT:
>>> -      optab = atomic_bit_test_and_complement_optab;
>>> -      break;
>>> -    case IFN_ATOMIC_BIT_TEST_AND_RESET:
>>> -      optab = atomic_bit_test_and_reset_optab;
>>> -      break;
>>> -    default:
>>> -      return;
>>> -    }
>>> -
>>>      if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs))) == CODE_FOR_nothing)
>>>        return;
>> Shouldn't the test of the return value of optab_handler here just go
>> away since we're testing it earlier?  OK with that fix.
>>
> The earlier check is predicated on if (rhs_code != BIT_AND_EXPR):
>
>    if (rhs_code != BIT_AND_EXPR)
>      {
>        if (rhs_code != NOP_EXPR && rhs_code != BIT_NOT_EXPR)
>          return;
>
>        tree use_lhs = gimple_assign_lhs (use_stmt);
>        if (TREE_CODE (use_lhs) == SSA_NAME
>            && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (use_lhs))
>          return;
>
>        tree use_rhs = gimple_assign_rhs1 (use_stmt);
>        if (lhs != use_rhs)
>          return;
>
>        if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs)))
>            == CODE_FOR_nothing)
>          return;
>
> I can add an "else"
>
> else  if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs)))
>              == CODE_FOR_nothing)
>      return;
>
> Will it be OK?
Sure.  THanks.
jeff
H.J. Lu Nov. 15, 2021, 8:58 p.m. UTC | #4 
On Mon, Nov 15, 2021 at 11:14 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 11/15/2021 12:05 PM, H.J. Lu wrote:
> > On Mon, Nov 15, 2021 at 10:59 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
> >>
> >>
> >> On 11/15/2021 6:39 AM, H.J. Lu via Gcc-patches wrote:
> >>> Check optab before transforming equivalent, but slighly different cases
> >>> of atomic bit test and operations to their canonical forms.
> >>>
> >>> gcc/
> >>>
> >>>        PR middle-end/103184
> >>>        * tree-ssa-ccp.c (optimize_atomic_bit_test_and): Check optab
> >>>        before transforming equivalent, but slighly different cases to
> >>>        their canonical forms.
> >>>
> >>> gcc/testsuite/
> >>>
> >>>        PR middle-end/103184
> >>>        * gcc.dg/pr103184-1.c: New test.
> >>>        * gcc.dg/pr103184-2.c: Likewise.
> >>>        }
> >>>        }
> >>>
> >>> -  switch (fn)
> >>> -    {
> >>> -    case IFN_ATOMIC_BIT_TEST_AND_SET:
> >>> -      optab = atomic_bit_test_and_set_optab;
> >>> -      break;
> >>> -    case IFN_ATOMIC_BIT_TEST_AND_COMPLEMENT:
> >>> -      optab = atomic_bit_test_and_complement_optab;
> >>> -      break;
> >>> -    case IFN_ATOMIC_BIT_TEST_AND_RESET:
> >>> -      optab = atomic_bit_test_and_reset_optab;
> >>> -      break;
> >>> -    default:
> >>> -      return;
> >>> -    }
> >>> -
> >>>      if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs))) == CODE_FOR_nothing)
> >>>        return;
> >> Shouldn't the test of the return value of optab_handler here just go
> >> away since we're testing it earlier?  OK with that fix.
> >>
> > The earlier check is predicated on if (rhs_code != BIT_AND_EXPR):
> >
> >    if (rhs_code != BIT_AND_EXPR)
> >      {
> >        if (rhs_code != NOP_EXPR && rhs_code != BIT_NOT_EXPR)
> >          return;
> >
> >        tree use_lhs = gimple_assign_lhs (use_stmt);
> >        if (TREE_CODE (use_lhs) == SSA_NAME
> >            && SSA_NAME_OCCURS_IN_ABNORMAL_PHI (use_lhs))
> >          return;
> >
> >        tree use_rhs = gimple_assign_rhs1 (use_stmt);
> >        if (lhs != use_rhs)
> >          return;
> >
> >        if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs)))
> >            == CODE_FOR_nothing)
> >          return;
> >
> > I can add an "else"
> >
> > else  if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs)))
> >              == CODE_FOR_nothing)
> >      return;
> >
> > Will it be OK?
> Sure.  THanks.
> jeff

This is the patch I am checking in.

Thanks.
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.dg/pr103184-1.c b/gcc/testsuite/gcc.dg/pr103184-1.c
new file mode 100644
index 00000000000..e567f95f63f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr103184-1.c
@@ -0,0 +1,43 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+extern char foo;
+extern unsigned char bar;
+
+int
+foo1 (void)
+{
+  return __sync_fetch_and_and (&foo, ~1) & 1;
+}
+
+int
+foo2 (void)
+{
+  return __sync_fetch_and_or (&foo, 1) & 1;
+}
+
+int
+foo3 (void)
+{
+  return __sync_fetch_and_xor (&foo, 1) & 1;
+}
+
+unsigned short
+bar1 (void)
+{
+  return __sync_fetch_and_and (&bar, ~1) & 1;
+}
+
+unsigned short
+bar2 (void)
+{
+  return __sync_fetch_and_or (&bar, 1) & 1;
+}
+
+unsigned short
+bar3 (void)
+{
+  return __sync_fetch_and_xor (&bar, 1) & 1;
+}
+
+/* { dg-final { scan-assembler-times "lock;?\[ \t\]*cmpxchgb" 6 { target { x86_64-*-* i?86-*-* } } } } */
diff --git a/gcc/testsuite/gcc.dg/pr103184-2.c b/gcc/testsuite/gcc.dg/pr103184-2.c
new file mode 100644
index 00000000000..499761fdbfd
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr103184-2.c
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#include <stdatomic.h>
+
+int
+tbit0 (_Atomic int* a, int n)
+{
+#define BIT (0x1 << n)
+  return atomic_fetch_or (a, BIT) & BIT;
+#undef BIT
+}
diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index 0f79e9f05bd..fec68b5fc73 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -3366,6 +3366,21 @@  optimize_atomic_bit_test_and (gimple_stmt_iterator *gsip,
       || !gimple_vdef (call))
     return;
 
+  switch (fn)
+    {
+    case IFN_ATOMIC_BIT_TEST_AND_SET:
+      optab = atomic_bit_test_and_set_optab;
+      break;
+    case IFN_ATOMIC_BIT_TEST_AND_COMPLEMENT:
+      optab = atomic_bit_test_and_complement_optab;
+      break;
+    case IFN_ATOMIC_BIT_TEST_AND_RESET:
+      optab = atomic_bit_test_and_reset_optab;
+      break;
+    default:
+      return;
+    }
+
   tree bit = nullptr;
 
   mask = gimple_call_arg (call, 1);
@@ -3384,6 +3399,10 @@  optimize_atomic_bit_test_and (gimple_stmt_iterator *gsip,
       if (lhs != use_rhs)
 	return;
 
+      if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs)))
+	  == CODE_FOR_nothing)
+	return;
+
       gimple *g;
       gimple_stmt_iterator gsi;
       tree var;
@@ -3628,21 +3647,6 @@  optimize_atomic_bit_test_and (gimple_stmt_iterator *gsip,
 	}
     }
 
-  switch (fn)
-    {
-    case IFN_ATOMIC_BIT_TEST_AND_SET:
-      optab = atomic_bit_test_and_set_optab;
-      break;
-    case IFN_ATOMIC_BIT_TEST_AND_COMPLEMENT:
-      optab = atomic_bit_test_and_complement_optab;
-      break;
-    case IFN_ATOMIC_BIT_TEST_AND_RESET:
-      optab = atomic_bit_test_and_reset_optab;
-      break;
-    default:
-      return;
-    }
-
   if (optab_handler (optab, TYPE_MODE (TREE_TYPE (lhs))) == CODE_FOR_nothing)
     return;