| Message ID | 20211115133943.440104-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Check optab before transforming atomic bit test and operations | expand |
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
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.
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
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 --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;