On 01/03/2018 12:08 AM, Prathamesh Kulkarni wrote:
> On 3 January 2018 at 12:33, Prathamesh Kulkarni
> <prathamesh.kulka...@linaro.org> wrote:
>> On 2 January 2018 at 17:49, Jakub Jelinek <ja...@redhat.com> wrote:
>>> On Tue, Jan 02, 2018 at 05:39:17PM +0530, Prathamesh Kulkarni wrote:
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr82665.c
>>>> @@ -0,0 +1,22 @@
>>>> +/* { dg-do compile } */
>>>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>>>> +
>>>> +void f1 (char *p, __SIZE_TYPE__ sz)
>>>> +{
>>>> + char *q = __builtin_memchr (p, 0, sz);
>>>> + long n = q - p;
>>> Please use __PTRDIFF_TYPE__ instead of long, here and in other spots.
>>>
>>>> --- a/gcc/vr-values.c
>>>> +++ b/gcc/vr-values.c
>>>> @@ -793,6 +793,42 @@ vr_values::extract_range_from_binary_expr
>>>> (value_range *vr,
>>>>
>>>> extract_range_from_binary_expr_1 (vr, code, expr_type, &vr0, &vr1);
>>>>
>>>> + /* Set value_range for n in following sequence:
>>>> + def = __builtin_memchr (arg, 0, sz)
>>>> + n = def - arg
>>>> + Here the range for n can be set to [0, PTRDIFF_MAX - 1]. */
>>>> +
>>>> + if (vr->type == VR_VARYING
>>>> + && (code == POINTER_DIFF_EXPR)
>>>> + && (TREE_CODE (op0) == SSA_NAME)
>>>> + && (TREE_CODE (op1) == SSA_NAME))
>>> Please drop the useless ()s around the comparisons. They are needed
>>> only if the condition is multi-line and needed for emacs indentation,
>>> or around assignments tested as conditions.
>>>
>>>> + gcall *call_stmt = NULL;
>>>> + if (def && arg
>>>> + && (TREE_CODE (def) == SSA_NAME)
>>> Likewise (many times).
>>>
>>>> + && ((TREE_CODE (TREE_TYPE (def)) == POINTER_TYPE)
>>>> + && (TREE_TYPE (TREE_TYPE (def)) == char_type_node))
>>>> + && (TREE_CODE (arg) == SSA_NAME)
>>>> + && ((TREE_CODE (TREE_TYPE (arg)) == POINTER_TYPE)
>>>> + && (TREE_TYPE (TREE_TYPE (arg)) == char_type_node))
>>> What is so special about char_type_node? Why e.g. unsigned char or signed
>>> char pointer shouldn't be handled the same?
>>>
>>>> + && (call_stmt = dyn_cast<gcall *>(SSA_NAME_DEF_STMT (def)))
>>>> + && (gimple_call_combined_fn (call_stmt) == CFN_BUILT_IN_MEMCHR)
>>> We don't have an ifn for memchr, so why this? On the other side, it should
>>> be making sure one doesn't use unprototyped memchr with weirdo argument
>>> types, so you need gimple_call_builtin_p.
>> Hi Jakub,
>> Thanks for the review. I have tried to address your suggestions in the
>> attached patch.
>> Does it look OK ?
>> Validation in progress.
> Oops, I mistakenly made argument sz in the tests of type
> __PTRDIFF_TYPE__ in the previous patch.
> The attached patch restores it's type to __SIZE_TYPE__.
>
> Thanks,
> Prathamesh
>> Thanks,
>> Prathamesh
>>>> + && operand_equal_p (def, gimple_call_lhs (call_stmt), 0)
>>>> + && operand_equal_p (arg, gimple_call_arg (call_stmt, 0), 0)
>>>> + && integer_zerop (gimple_call_arg (call_stmt, 1)))
>>>> + {
>>>> + tree max = vrp_val_max (ptrdiff_type_node);
>>>> + wide_int wmax = wi::to_wide (max, TYPE_PRECISION (TREE_TYPE
>>>> (max)));
>>>> + tree range_min = build_zero_cst (expr_type);
>>>> + tree range_max = wide_int_to_tree (expr_type, wmax - 1);
>>>> + set_value_range (vr, VR_RANGE, range_min, range_max, NULL);
>>>> + return;
>>>> + }
>>>> + }
>>>> +
>>>> /* Try harder for PLUS and MINUS if the range of one operand is symbolic
>>>> and based on the other operand, for example if it was deduced from a
>>>> symbolic comparison. When a bound of the range of the first operand
>>>
>>> Jakub
>
> pr82665-8.diff
>
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr82665.c
> b/gcc/testsuite/gcc.dg/tree-ssa/pr82665.c
> new file mode 100644
> index 00000000000..b37c3d1d7ce
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/pr82665.c
> @@ -0,0 +1,32 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +void f1 (char *p, __SIZE_TYPE__ sz)
> +{
> + char *q = __builtin_memchr (p, 0, sz);
> + __PTRDIFF_TYPE__ n = q - p;
> +
> + if (n >= __PTRDIFF_MAX__)
> + __builtin_abort ();
> +}
> +
> +void f2 (unsigned char *p, __SIZE_TYPE__ sz)
> +{
> + unsigned char *q = __builtin_memchr (p, 0, sz);
> + __PTRDIFF_TYPE__ n = q - p;
> +
> + if (n >= __PTRDIFF_MAX__)
> + __builtin_abort ();
> +}
> +
> +void f3 (signed char *p, __SIZE_TYPE__ sz)
> +{
> + signed char *q = __builtin_memchr (p, 0, sz);
> + __PTRDIFF_TYPE__ n = q - p;
> +
> + if (n >= __PTRDIFF_MAX__)
> + __builtin_abort ();
> +}
> +
> +
> +/* { dg-final { scan-tree-dump-not "memchr" 0 "optimized" } } */
> diff --git a/gcc/vr-values.c b/gcc/vr-values.c
> index 794b4635f9e..2c93f92438a 100644
> --- a/gcc/vr-values.c
> +++ b/gcc/vr-values.c
> @@ -793,6 +793,47 @@ vr_values::extract_range_from_binary_expr (value_range
> *vr,
>
> extract_range_from_binary_expr_1 (vr, code, expr_type, &vr0, &vr1);
>
> + /* Set value_range for n in following sequence:
> + def = __builtin_memchr (arg, 0, sz)
> + n = def - arg
> + Here the range for n can be set to [0, PTRDIFF_MAX - 1]. */
> +
> + if (vr->type == VR_VARYING
> + && code == POINTER_DIFF_EXPR
> + && TREE_CODE (op0) == SSA_NAME
> + && TREE_CODE (op1) == SSA_NAME)
> + {
> + tree def = op0;
> + tree arg = op1;
> + tree def_type, arg_type;
> +
> + gcall *call_stmt = NULL;
> + if (def && arg
AFAICT these can never be NULL at this point. They are op0 and op1
respectively and we've verified they are SSA_NAMEs. So I think this
test is redundant and should be removed.
> + && TREE_CODE (def) == SSA_NAME
Similarly this test is redundant with verification that op0 is an
SSA_NAME in the outer conditional.
> + && TREE_CODE (TREE_TYPE (def)) == POINTER_TYPE
> + && (def_type = TREE_TYPE (TREE_TYPE (def)))
> + && (def_type == char_type_node || def_type == signed_char_type_node
> + || def_type == unsigned_char_type_node)
Why are we checking for equality with these types at all. Aren't we
going to miss equivalent types or types with qualifiers?
It looks like the canonical way to do this check in tree-ssa-strlen.c is
(TYPE_MODE (type) == TYPE_MODE (char_type_node)
&& TYPE_PRECISION (type) == TYPE_PRECISION (char_type_node))
ie, verify that the mode/precision of the object is the same as the
mode/precision of a character type.
> + && TREE_CODE (arg) == SSA_NAME
Redundant with the verification that op1 is an SSA_NAME in the outer
conditional.
> + && TREE_CODE (TREE_TYPE (arg)) == POINTER_TYPE
> + && (arg_type = TREE_TYPE (TREE_TYPE (arg)))
> + && (arg_type == char_type_node || arg_type == signed_char_type_node
> + || arg_type == unsigned_char_type_node)
See note above about verifying based on mode/precision.
I think with those changes we're probably in good shape. But please
repost for final approval.
jeff
