On 5 January 2018 at 00:20, Jeff Law <[email protected]> wrote:
> On 01/03/2018 12:08 AM, Prathamesh Kulkarni wrote:
>> On 3 January 2018 at 12:33, Prathamesh Kulkarni
>> <[email protected]> wrote:
>>> On 2 January 2018 at 17:49, Jakub Jelinek <[email protected]> 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.
Indeed. These checks were carried over from my previous patch before
basing on POINTER_DIFF_EXPR,
and I forgot to remove them later :/
>
>
>> + && 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.
Thanks, I updated the patch to check for mode/precision.
>
>
>> + && 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.
I have the updated the attached version with your suggestions.
Does it look OK ?
Bootstrap+test passes on x86_64-unknown-linux-gnu.
Thanks,
Prathamesh
>
> jeff
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..66db32f46db
--- /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" "optimized" } } */
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 794b4635f9e..41a4a0b041f 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -793,6 +793,39 @@ 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 op0_ptype = TREE_TYPE (TREE_TYPE (op0));
+ tree op1_ptype = TREE_TYPE (TREE_TYPE (op1));
+ gcall *call_stmt = NULL;
+
+ if (TYPE_MODE (op0_ptype) == TYPE_MODE (char_type_node)
+ && TYPE_PRECISION (op0_ptype) == TYPE_PRECISION (char_type_node)
+ && TYPE_MODE (op1_ptype) == TYPE_MODE (char_type_node)
+ && TYPE_PRECISION (op1_ptype) == TYPE_PRECISION (char_type_node)
+ && (call_stmt = dyn_cast<gcall *>(SSA_NAME_DEF_STMT (op0)))
+ && gimple_call_builtin_p (call_stmt, BUILT_IN_MEMCHR)
+ && operand_equal_p (op0, gimple_call_lhs (call_stmt), 0)
+ && operand_equal_p (op1, 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
