https://gcc.gnu.org/g:44c89620f7390687a01045228d263845e6e311aa
commit r17-3822-g44c89620f7390687a01045228d263845e6e311aa Author: Andrea Pinski <[email protected]> Date: Sat Aug 29 11:46:17 2026 -0700 phiopt: Fix factor/merging out of calls [PR126977] Some builtins and some expressions can't be combined in some cases. An example is the crc builtins can't be factored out/commonialized if the 3rd argument is different as that would cause an error as the crc builtins require a constant there. The same is true of some others like BIT_INSERT_EXPR and BIT_FIELD_REF which requires a constant. Right now there are 3 different locations which handle the same thing. This extracts all 3 different locations into one function, factor_operation_ok. And adds a few more restrictions that was not handled before: CRC, expect, object_size, clz, ctz (only internal function with 2 arguments), prefetch, and frame address. Note since the restriction on BIT_INSERT_EXPR is relaxed slightly, pr113609-2.c needs to be updated to allow for the better optimization that is happening now. PR tree-optimization/126977 gcc/ChangeLog: * gimple-match-exports.cc (fn_arg_must_be_const_p): New function. (all_ssa_names_p): New function. (factor_operation_ok): New exported function. * gimple-match.h (factor_operation_ok): New declaration. * tree-if-conv.cc (factor_out_operators): Use factor_operation_ok. * tree-ssa-phiopt.cc (factor_out_conditional_operation): Use factor_operation_ok. * tree-ssa-tail-merge.cc (merge_stmts_p): Use factor_operation_ok instead of just looking at specific internal functions. gcc/testsuite/ChangeLog: * gcc.target/i386/pr113609-2.c: Update for factoring of BIT_INSERT_EXPR. * gcc.dg/torture/pr126977-1.c: New test. Signed-off-by: Andrea Pinski <[email protected]> Co-authored-by: Naveen <[email protected]> Diff: --- gcc/gimple-match-exports.cc | 167 +++++++++++++++++++++++++++++ gcc/gimple-match.h | 3 + gcc/testsuite/gcc.dg/torture/pr126977-1.c | 74 +++++++++++++ gcc/testsuite/gcc.target/i386/pr113609-2.c | 4 +- gcc/tree-if-conv.cc | 26 +---- gcc/tree-ssa-phiopt.cc | 47 +------- gcc/tree-ssa-tail-merge.cc | 29 ++--- 7 files changed, 266 insertions(+), 84 deletions(-) diff --git a/gcc/gimple-match-exports.cc b/gcc/gimple-match-exports.cc index 74f2c5f19bcf..8077bbad165a 100644 --- a/gcc/gimple-match-exports.cc +++ b/gcc/gimple-match-exports.cc @@ -1492,3 +1492,170 @@ commutative: } return -1; } + +/* Returns true if CFN requires a constant at ARGNO argument. */ + +static bool +fn_arg_must_be_const_p (combined_fn cfn, int argno) +{ + gcc_checking_assert (argno >= 0); + internal_fn ifn; + if (internal_fn_p (cfn)) + { + ifn = as_internal_fn (cfn); + if (argno == 2 + && (ifn == IFN_CRC || ifn == IFN_CRC_REV)) + return true; + if (argno == 1 + && (ifn == IFN_CLZ || ifn == IFN_CTZ)) + return true; + if (ifn == IFN_BUILTIN_EXPECT) + return true; + return false; + } + switch (as_builtin_fn (cfn)) + { + case BUILT_IN_CRC8_DATA8: + case BUILT_IN_CRC16_DATA8: + case BUILT_IN_CRC16_DATA16: + case BUILT_IN_CRC32_DATA8: + case BUILT_IN_CRC32_DATA16: + case BUILT_IN_CRC32_DATA32: + case BUILT_IN_CRC64_DATA8: + case BUILT_IN_CRC64_DATA16: + case BUILT_IN_CRC64_DATA32: + case BUILT_IN_CRC64_DATA64: + case BUILT_IN_REV_CRC8_DATA8: + case BUILT_IN_REV_CRC16_DATA8: + case BUILT_IN_REV_CRC16_DATA16: + case BUILT_IN_REV_CRC32_DATA8: + case BUILT_IN_REV_CRC32_DATA16: + case BUILT_IN_REV_CRC32_DATA32: + case BUILT_IN_REV_CRC64_DATA8: + case BUILT_IN_REV_CRC64_DATA16: + case BUILT_IN_REV_CRC64_DATA32: + case BUILT_IN_REV_CRC64_DATA64: + return argno == 2; + case BUILT_IN_OBJECT_SIZE: + return argno == 1; + case BUILT_IN_PREFETCH: + return argno != 0; + case BUILT_IN_FRAME_ADDRESS: + return true; + case BUILT_IN_EXPECT_WITH_PROBABILITY: + case BUILT_IN_EXPECT: + return true; + default: + return false; + } + return false; +} + +/* Returns true if the array ARGS (size NUM_ARGS) are all ssa names. */ +static bool +all_ssa_names_p (tree *args, size_t num_args) +{ + for (size_t i = 0; i < num_args; i++) + if (TREE_CODE (args[i]) != SSA_NAME) + return false; + return true; +} + +/* Some factoring of operations including some builtins need + to be stop from happening in some cases. factor_operation_ok + returns true when the factoring is ok for CODE where OPNUM is + operand was and the ARGS are the operands of the + original operation. DIVCONSTOK says if an integer division with + a constant is ok to be factored out. PTRPLUSCONSTOK says if a + POINTER_PLUS_EXPR can be factored out. When opnum < 0, args + is allowed to be null. */ + +bool +factor_operation_ok (code_helper code, int opnum, + tree *args, location_t *locs, size_t num_args, + bool divconstok, bool ptrplusconstok) +{ + bool all_ssa_names = opnum < 0 ? true : all_ssa_names_p (args, num_args); + + if (code.is_fn_code () + && code.is_internal_fn ()) + switch (internal_fn (code)) + { + /* For these internal functions, gimple_location is an implicit + parameter, which will be used explicitly after expansion. + Merging these statements may cause confusing line numbers in + sanitizer messages. */ + case IFN_UBSAN_NULL: + case IFN_UBSAN_BOUNDS: + case IFN_UBSAN_VPTR: + case IFN_UBSAN_CHECK_ADD: + case IFN_UBSAN_CHECK_SUB: + case IFN_UBSAN_CHECK_MUL: + case IFN_UBSAN_OBJECT_SIZE: + case IFN_UBSAN_PTR: + case IFN_ASAN_CHECK: + for (size_t i = 1; i < num_args; i++) + if (locs[0] != locs[i]) + return false; + break; + default: ; + } + + /* If this was a division and the operand is the divisor + and either divisor was a constant, don't factor out + the division; dividing by an explicit constant can be + expanded better than without an constant. + FIXME: maybe isel could undo this case. */ + if (!all_ssa_names + && !divconstok + && int_divide_or_mod_p (code) + && opnum == 1) + return false; + + /* For early phiopt, don't factor out constants for pointer plus. + BOS pass does not like that factoring. */ + if (!all_ssa_names + && !ptrplusconstok && code == POINTER_PLUS_EXPR + && opnum == 1) + return false; + + /* BIT_FIELD_REF can't factor out operand 1 and 2. + While BIT_INSERT_EXPR can't be factored out for operand 2. + These operands require constants. */ + if (!all_ssa_names + && code == BIT_FIELD_REF + && (opnum == 1 || opnum == 2)) + return false; + + if (!all_ssa_names + && code == BIT_INSERT_EXPR + && opnum == 2) + return false; + + /* It is not profitability to factor out vec_perm with + constant masks (operand 2). The target might not support it + and that might be invalid to do as such. Also with constants + masks, the number of elements of the mask type does not need + to match the number of elements of other operands and can be + arbitrary integral vector type so factoring that out can't work. + Note in the case where one mask is a constant and the other is not, + the check for compatible types will reject the case the + constant mask has the incompatible type. */ + if (!all_ssa_names + && code == VEC_PERM_EXPR && opnum == 2) + return false; + + /* Some builtins should not be factored out with constants being involved. */ + if (!all_ssa_names && code.is_fn_code ()) + return !fn_arg_must_be_const_p (combined_fn (code), opnum); + + // The types are need to be compatible. + if (opnum >= 0) + { + for (size_t i = 1; i < num_args; i++) + if (!types_compatible_p (TREE_TYPE (args[0]), TREE_TYPE (args[1]))) + return false; + } + + return true; +} diff --git a/gcc/gimple-match.h b/gcc/gimple-match.h index 8ff58e8cd7f2..fb5cfe7a5624 100644 --- a/gcc/gimple-match.h +++ b/gcc/gimple-match.h @@ -429,5 +429,8 @@ internal_fn get_conditional_internal_fn (code_helper, tree); int find_different_opnum (const gimple_match_op &arg0_op, const gimple_match_op &arg1_op, tree *new_arg0, tree *new_arg1); +bool factor_operation_ok (code_helper code, int opnum, + tree *args, location_t *locs, size_t numargs, + bool divconstok, bool ptrplusconstok); #endif /* GCC_GIMPLE_MATCH_H */ diff --git a/gcc/testsuite/gcc.dg/torture/pr126977-1.c b/gcc/testsuite/gcc.dg/torture/pr126977-1.c new file mode 100644 index 000000000000..141142c869c5 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr126977-1.c @@ -0,0 +1,74 @@ +/* { dg-do compile } */ +/* PR tree-optimization/126977 */ + +/* All of these should be valid and should not cause + any errors even with factoring happening. */ + +typedef __UINT32_TYPE__ u32; + +u32 crc32_data32 (u32 x, u32 y, int z) +{ + if (z) + return __builtin_crc32_data32 (x, y, 0x4002123); + else + return __builtin_crc32_data32 (x, y, 0x4002124); +} + +int bos (int z, void *p) +{ + if (z) + return __builtin_object_size (p, 0); + else + return __builtin_object_size (p, 1); +} + + +void prefetch (unsigned *x, int z) +{ + if (z) + __builtin_prefetch (x, 1); + else + __builtin_prefetch (x, 0); +} +void *faddr (unsigned *x, int z) +{ + if (z) + return __builtin_frame_address (1); + else + return __builtin_frame_address (0); +} + + +int expect_issue (long a, int z) +{ + if (z) + return __builtin_expect (a, 1); + else + return __builtin_expect (a, 0); +} + + +struct f +{ + int a[2]; +}; +struct f1 +{ + char a[2]; +}; +int islock (long a, int z, void *p) +{ + if (z) + return __atomic_is_lock_free (sizeof(struct f), p); + else + return __atomic_is_lock_free (sizeof(struct f1), p); +} + + +int clz1 (int z, unsigned int p) +{ + if (z) + return __builtin_clzg (p, 0); + else + return __builtin_clzg (p, 32); +} diff --git a/gcc/testsuite/gcc.target/i386/pr113609-2.c b/gcc/testsuite/gcc.target/i386/pr113609-2.c index e9503f51538b..ba8fa38866b8 100644 --- a/gcc/testsuite/gcc.target/i386/pr113609-2.c +++ b/gcc/testsuite/gcc.target/i386/pr113609-2.c @@ -1,9 +1,9 @@ /* PR target/113609 */ /* { dg-do compile } */ /* { dg-options "-O2 -march=x86-64-v4" } */ -/* { dg-final { scan-assembler-times "\[ \\t\]+sete" 4 } } */ +/* { dg-final { scan-assembler-times "\[ \\t\]+sete" 8 } } */ /* { dg-final { scan-assembler-times "\[ \\t\]+setne" 4 } } */ -/* { dg-final { scan-assembler-times "\[ \\t\]+je" 4 } } */ +/* { dg-final { scan-assembler-times "\[ \\t\]+je" 0 } } */ /* { dg-final { scan-assembler-times "\[ \\t\]+jne" 4 } } */ #include <immintrin.h> diff --git a/gcc/tree-if-conv.cc b/gcc/tree-if-conv.cc index 200e2b165abb..fe749f94bd0c 100644 --- a/gcc/tree-if-conv.cc +++ b/gcc/tree-if-conv.cc @@ -2226,29 +2226,13 @@ again: if (opnum == -1) return; - /* BIT_FIELD_REF and BIT_INSERT_EXPR can't be factored out for non-0 operands - as the other operands require constants. */ - if ((arg1_op.code == BIT_FIELD_REF - || arg1_op.code == BIT_INSERT_EXPR) - && opnum != 0) + tree args[2] = { new_arg0, new_arg1 }; + location_t locs[2]; + locs[0] = gimple_location (arg0_def_stmt); + locs[1] = gimple_location (arg1_def_stmt); + if (!factor_operation_ok (arg1_op.code, opnum, args, locs, 2, true, true)) return; - /* It is not profitability to factor out vec_perm with - constant masks (operand 2). The target might not support it - and that might be invalid to do as such. Also with constants - masks, the number of elements of the mask type does not need - to match the number of elements of other operands and can be - arbitrary integral vector type so factoring that out can't work. - Note in the case where one mask is a constant and the other is not, - the next check for compatible types will reject the case the - constant mask has the incompatible type. */ - if (arg1_op.code == VEC_PERM_EXPR && opnum == 2 - && TREE_CODE (new_arg0) == VECTOR_CST - && TREE_CODE (new_arg1) == VECTOR_CST) - return; - - if (!types_compatible_p (TREE_TYPE (new_arg0), TREE_TYPE (new_arg1))) - return; tree new_res = make_ssa_name (TREE_TYPE (new_arg0), NULL); /* Create the operation stmt if possible and insert it. */ diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc index e9037e78e287..959ab66133db 100644 --- a/gcc/tree-ssa-phiopt.cc +++ b/gcc/tree-ssa-phiopt.cc @@ -430,44 +430,11 @@ factor_out_conditional_operation (edge e0, edge e1, basic_block merge, arg1_op.ops, arg1_op.num_ops)) return false; - /* If this was a division and the operand is the divisor - and either divisor was a constant, don't factor out - the division; dividing by an explicit constant can be - expanded better than without an constant. - FIXME: maybe isel could undo this case. */ - if (int_divide_or_mod_p (arg1_op.code) - && opnum == 1 - && (poly_int_tree_p (new_arg0) - || poly_int_tree_p (new_arg1))) - return false; - - /* For early phiopt, don't factor out constants for pointer plus. - BOS pass does not like that factoring. */ - if (early_p && arg1_op.code == POINTER_PLUS_EXPR - && opnum == 1 - && TREE_CODE (new_arg0) != SSA_NAME - && TREE_CODE (new_arg1) != SSA_NAME) - return false; - - /* BIT_FIELD_REF and BIT_INSERT_EXPR can't be factored out for non-0 operands - as the other operands require constants. */ - if ((arg1_op.code == BIT_FIELD_REF - || arg1_op.code == BIT_INSERT_EXPR) - && opnum != 0) - return false; - - /* It is not profitability to factor out vec_perm with - constant masks (operand 2). The target might not support it - and that might be invalid to do as such. Also with constants - masks, the number of elements of the mask type does not need - to match the number of elements of other operands and can be - arbitrary integral vector type so factoring that out can't work. - Note in the case where one mask is a constant and the other is not, - the check for compatible types will reject the case the - constant mask has the incompatible type. */ - if (arg1_op.code == VEC_PERM_EXPR && opnum == 2 - && TREE_CODE (new_arg0) == VECTOR_CST - && TREE_CODE (new_arg1) == VECTOR_CST) + tree args[2] = { new_arg0, new_arg1 }; + location_t locs[2]; + locs[0] = gimple_location (arg0_def_stmt); + locs[1] = gimple_location (arg1_def_stmt); + if (!factor_operation_ok (arg1_op.code, opnum, args, locs, 2, false, !early_p)) return false; if (gimple_has_location (arg1_def_stmt)) @@ -575,10 +542,6 @@ factor_out_conditional_operation (edge e0, edge e1, basic_block merge, locus = gimple_location (arg0_def_stmt); } - /* If types of new_arg0 and new_arg1 are different bailout. */ - if (!types_compatible_p (TREE_TYPE (new_arg0), TREE_TYPE (new_arg1))) - return false; - /* Create a new PHI stmt. */ result = gimple_phi_result (phi); temp = make_ssa_name (TREE_TYPE (new_arg0), NULL); diff --git a/gcc/tree-ssa-tail-merge.cc b/gcc/tree-ssa-tail-merge.cc index 982ce5cb37f9..6d81a7118b32 100644 --- a/gcc/tree-ssa-tail-merge.cc +++ b/gcc/tree-ssa-tail-merge.cc @@ -207,6 +207,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-eh.h" #include "tree-cfgcleanup.h" #include "tree-ssa.h" +#include "gimple-match.h" const int ignore_edge_flags = EDGE_DFS_BACK | EDGE_EXECUTABLE; @@ -1320,25 +1321,15 @@ merge_stmts_p (gimple *stmt1, gimple *stmt2) if (is_gimple_call (stmt1) && gimple_call_internal_p (stmt1)) - switch (gimple_call_internal_fn (stmt1)) - { - case IFN_UBSAN_NULL: - case IFN_UBSAN_BOUNDS: - case IFN_UBSAN_VPTR: - case IFN_UBSAN_CHECK_ADD: - case IFN_UBSAN_CHECK_SUB: - case IFN_UBSAN_CHECK_MUL: - case IFN_UBSAN_OBJECT_SIZE: - case IFN_UBSAN_PTR: - case IFN_ASAN_CHECK: - /* For these internal functions, gimple_location is an implicit - parameter, which will be used explicitly after expansion. - Merging these statements may cause confusing line numbers in - sanitizer messages. */ - return gimple_location (stmt1) == gimple_location (stmt2); - default: - break; - } + { + location_t locs[2]; + locs[0] = gimple_location (stmt1); + locs[1] = gimple_location (stmt2); + + if (!factor_operation_ok (gimple_call_internal_fn (stmt1), + -2, nullptr, locs, 2, true, true)) + return false; + } return true; }
