| Message ID | 87io5eq8nv.fsf@e105548-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
On 11/07/2015 05:46 AM, Richard Sandiford wrote: > Another patch to extend uses of built_in_function to combined_fn, this time > in tree-vrp.c. > > Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi. > OK to install? > > Thanks, > Richard > > > gcc/ > * tree-vrp.c: Include case-cfn-macros.h. > (extract_range_basic): Switch on combined_fn rather than handling > built-in functions and internal functions separately. OK. jeff
On 11/07/2015 01:46 PM, Richard Sandiford wrote: > @@ -3814,8 +3817,8 @@ extract_range_basic (value_range *vr, gimple *stmt) > break; > /* Both __builtin_ffs* and __builtin_popcount return > [0, prec]. */ > - CASE_INT_FN (BUILT_IN_FFS): > - CASE_INT_FN (BUILT_IN_POPCOUNT): > + CASE_CFN_FFS: > + CASE_CFN_POPCOUNT: > arg = gimple_call_arg (stmt, 0); > prec = TYPE_PRECISION (TREE_TYPE (arg)); > mini = 0; So let me see if I understood this. From what we discussed the purpose of these new internal functions is that they can have vector types. If so, isn't this code (here and elsewhere) which expects integers potentially going to be confused? Bernd
On Tue, Nov 10, 2015 at 1:09 AM, Bernd Schmidt <bschmidt@redhat.com> wrote: > On 11/07/2015 01:46 PM, Richard Sandiford wrote: >> >> @@ -3814,8 +3817,8 @@ extract_range_basic (value_range *vr, gimple *stmt) >> break; >> /* Both __builtin_ffs* and __builtin_popcount return >> [0, prec]. */ >> - CASE_INT_FN (BUILT_IN_FFS): >> - CASE_INT_FN (BUILT_IN_POPCOUNT): >> + CASE_CFN_FFS: >> + CASE_CFN_POPCOUNT: >> arg = gimple_call_arg (stmt, 0); >> prec = TYPE_PRECISION (TREE_TYPE (arg)); >> mini = 0; > > > So let me see if I understood this. From what we discussed the purpose of > these new internal functions is that they can have vector types. If so, > isn't this code (here and elsewhere) which expects integers potentially > going to be confused? We indeed need to add additional checks to most users of CASE_CFN_* to cover the bigger freedom that exists with respect to types. Richard, please audit all the cases you change for that. Thanks, Richard. > > > Bernd
Richard Biener <richard.guenther@gmail.com> writes: > On Tue, Nov 10, 2015 at 1:09 AM, Bernd Schmidt <bschmidt@redhat.com> wrote: >> On 11/07/2015 01:46 PM, Richard Sandiford wrote: >>> >>> @@ -3814,8 +3817,8 @@ extract_range_basic (value_range *vr, gimple *stmt) >>> break; >>> /* Both __builtin_ffs* and __builtin_popcount return >>> [0, prec]. */ >>> - CASE_INT_FN (BUILT_IN_FFS): >>> - CASE_INT_FN (BUILT_IN_POPCOUNT): >>> + CASE_CFN_FFS: >>> + CASE_CFN_POPCOUNT: >>> arg = gimple_call_arg (stmt, 0); >>> prec = TYPE_PRECISION (TREE_TYPE (arg)); >>> mini = 0; >> >> >> So let me see if I understood this. From what we discussed the purpose of >> these new internal functions is that they can have vector types. If so, >> isn't this code (here and elsewhere) which expects integers potentially >> going to be confused? > > We indeed need to add additional checks to most users of CASE_CFN_* to cover > the bigger freedom that exists with respect to types. The code above is OK because it's only handling integral types. A vector popcount or vector ffs must return a vector result. > Richard, please audit all the cases you change for that. I had another look and the only problematical uses I could see are the match.pd ones. E.g.: /* Optimize logN(func()) for various exponential functions. We want to determine the value "x" and the power "exponent" in order to transform logN(x**exponent) into exponent*logN(x). */ (for logs (LOG LOG LOG LOG2 LOG2 LOG2 LOG10 LOG10) exps (EXP2 EXP10 POW10 EXP EXP10 POW10 EXP EXP2) (simplify (logs (exps @0)) (with { tree x; switch (exps) { CASE_CFN_EXP: /* Prepare to do logN(exp(exponent)) -> exponent*logN(e). */ x = build_real_truncate (type, dconst_e ()); break; CASE_CFN_EXP2: /* Prepare to do logN(exp2(exponent)) -> exponent*logN(2). */ x = build_real (type, dconst2); break; CASE_CFN_EXP10: CASE_CFN_POW10: /* Prepare to do logN(exp10(exponent)) -> exponent*logN(10). */ { REAL_VALUE_TYPE dconst10; real_from_integer (&dconst10, VOIDmode, 10, SIGNED); x = build_real (type, dconst10); } break; default: gcc_unreachable (); } } (mult (logs { x; }) @0)))) Here we could either add a SCALAR_FLOAT_TYPE_P check or extend build_real to handle vector types. Which do you think would be best? Thanks, Richard
On Fri, Nov 13, 2015 at 12:27 PM, Richard Sandiford <richard.sandiford@arm.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Tue, Nov 10, 2015 at 1:09 AM, Bernd Schmidt <bschmidt@redhat.com> wrote: >>> On 11/07/2015 01:46 PM, Richard Sandiford wrote: >>>> >>>> @@ -3814,8 +3817,8 @@ extract_range_basic (value_range *vr, gimple *stmt) >>>> break; >>>> /* Both __builtin_ffs* and __builtin_popcount return >>>> [0, prec]. */ >>>> - CASE_INT_FN (BUILT_IN_FFS): >>>> - CASE_INT_FN (BUILT_IN_POPCOUNT): >>>> + CASE_CFN_FFS: >>>> + CASE_CFN_POPCOUNT: >>>> arg = gimple_call_arg (stmt, 0); >>>> prec = TYPE_PRECISION (TREE_TYPE (arg)); >>>> mini = 0; >>> >>> >>> So let me see if I understood this. From what we discussed the purpose of >>> these new internal functions is that they can have vector types. If so, >>> isn't this code (here and elsewhere) which expects integers potentially >>> going to be confused? >> >> We indeed need to add additional checks to most users of CASE_CFN_* to cover >> the bigger freedom that exists with respect to types. > > The code above is OK because it's only handling integral types. > A vector popcount or vector ffs must return a vector result. > >> Richard, please audit all the cases you change for that. > > I had another look and the only problematical uses I could see are the > match.pd ones. E.g.: > > /* Optimize logN(func()) for various exponential functions. We > want to determine the value "x" and the power "exponent" in > order to transform logN(x**exponent) into exponent*logN(x). */ > (for logs (LOG LOG LOG LOG2 LOG2 LOG2 LOG10 LOG10) > exps (EXP2 EXP10 POW10 EXP EXP10 POW10 EXP EXP2) > (simplify > (logs (exps @0)) > (with { > tree x; > switch (exps) > { > CASE_CFN_EXP: > /* Prepare to do logN(exp(exponent)) -> exponent*logN(e). */ > x = build_real_truncate (type, dconst_e ()); > break; > CASE_CFN_EXP2: > /* Prepare to do logN(exp2(exponent)) -> exponent*logN(2). */ > x = build_real (type, dconst2); > break; > CASE_CFN_EXP10: > CASE_CFN_POW10: > /* Prepare to do logN(exp10(exponent)) -> exponent*logN(10). */ > { > REAL_VALUE_TYPE dconst10; > real_from_integer (&dconst10, VOIDmode, 10, SIGNED); > x = build_real (type, dconst10); > } > break; > default: > gcc_unreachable (); > } > } > (mult (logs { x; }) @0)))) > > Here we could either add a SCALAR_FLOAT_TYPE_P check or extend > build_real to handle vector types. Which do you think would be best? I don't like extending build_real. For now add a SCALAR_FLOAT_TYPE_P. Or maybe extend build_real ... Well. Go with SCALAR_FLOAT_TYPE_P. Thanks, Richard. > > Thanks, > Richard >
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index 87c0265..4b4179d 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -57,6 +57,7 @@ along with GCC; see the file COPYING3. If not see #include "tree-ssa-threadedge.h" #include "omp-low.h" #include "target.h" +#include "case-cfn-macros.h" /* Range of values that can be associated with an SSA_NAME after VRP has executed. */ @@ -3791,14 +3792,16 @@ extract_range_basic (value_range *vr, gimple *stmt) bool sop = false; tree type = gimple_expr_type (stmt); - if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)) + if (is_gimple_call (stmt)) { - tree fndecl = gimple_call_fndecl (stmt), arg; + tree arg; int mini, maxi, zerov = 0, prec; + enum tree_code subcode = ERROR_MARK; + combined_fn cfn = gimple_call_combined_fn (stmt); - switch (DECL_FUNCTION_CODE (fndecl)) + switch (cfn) { - case BUILT_IN_CONSTANT_P: + case CFN_BUILT_IN_CONSTANT_P: /* If the call is __builtin_constant_p and the argument is a function parameter resolve it to false. This avoids bogus array bound warnings. @@ -3814,8 +3817,8 @@ extract_range_basic (value_range *vr, gimple *stmt) break; /* Both __builtin_ffs* and __builtin_popcount return [0, prec]. */ - CASE_INT_FN (BUILT_IN_FFS): - CASE_INT_FN (BUILT_IN_POPCOUNT): + CASE_CFN_FFS: + CASE_CFN_POPCOUNT: arg = gimple_call_arg (stmt, 0); prec = TYPE_PRECISION (TREE_TYPE (arg)); mini = 0; @@ -3843,7 +3846,7 @@ extract_range_basic (value_range *vr, gimple *stmt) } goto bitop_builtin; /* __builtin_parity* returns [0, 1]. */ - CASE_INT_FN (BUILT_IN_PARITY): + CASE_CFN_PARITY: mini = 0; maxi = 1; goto bitop_builtin; @@ -3852,7 +3855,7 @@ extract_range_basic (value_range *vr, gimple *stmt) On many targets where the CLZ RTL or optab value is defined for 0 the value is prec, so include that in the range by default. */ - CASE_INT_FN (BUILT_IN_CLZ): + CASE_CFN_CLZ: arg = gimple_call_arg (stmt, 0); prec = TYPE_PRECISION (TREE_TYPE (arg)); mini = 0; @@ -3907,7 +3910,7 @@ extract_range_basic (value_range *vr, gimple *stmt) If there is a ctz optab for this mode and CTZ_DEFINED_VALUE_AT_ZERO, include that in the range, otherwise just assume 0 won't be seen. */ - CASE_INT_FN (BUILT_IN_CTZ): + CASE_CFN_CTZ: arg = gimple_call_arg (stmt, 0); prec = TYPE_PRECISION (TREE_TYPE (arg)); mini = 0; @@ -3956,7 +3959,7 @@ extract_range_basic (value_range *vr, gimple *stmt) break; goto bitop_builtin; /* __builtin_clrsb* returns [0, prec-1]. */ - CASE_INT_FN (BUILT_IN_CLRSB): + CASE_CFN_CLRSB: arg = gimple_call_arg (stmt, 0); prec = TYPE_PRECISION (TREE_TYPE (arg)); mini = 0; @@ -3966,33 +3969,22 @@ extract_range_basic (value_range *vr, gimple *stmt) set_value_range (vr, VR_RANGE, build_int_cst (type, mini), build_int_cst (type, maxi), NULL); return; - default: - break; - } - } - else if (is_gimple_call (stmt) && gimple_call_internal_p (stmt)) - { - enum tree_code subcode = ERROR_MARK; - unsigned ifn_code = gimple_call_internal_fn (stmt); - - switch (ifn_code) - { - case IFN_UBSAN_CHECK_ADD: + case CFN_UBSAN_CHECK_ADD: subcode = PLUS_EXPR; break; - case IFN_UBSAN_CHECK_SUB: + case CFN_UBSAN_CHECK_SUB: subcode = MINUS_EXPR; break; - case IFN_UBSAN_CHECK_MUL: + case CFN_UBSAN_CHECK_MUL: subcode = MULT_EXPR; break; - case IFN_GOACC_DIM_SIZE: - case IFN_GOACC_DIM_POS: + case CFN_GOACC_DIM_SIZE: + case CFN_GOACC_DIM_POS: /* Optimizing these two internal functions helps the loop optimizer eliminate outer comparisons. Size is [1,N] and pos is [0,N-1]. */ { - bool is_pos = ifn_code == IFN_GOACC_DIM_POS; + bool is_pos = cfn == CFN_GOACC_DIM_POS; int axis = get_oacc_ifn_dim_arg (stmt); int size = get_oacc_fn_dim_size (current_function_decl, axis);