Re: Fold strstr (s, t) eq/ne s to strcmp (s, t) eq/ne 0 if strlen (t) is known

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

On 7 December 2016 at 17:36, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 07, 2016 at 05:02:46PM +0530, Prathamesh Kulkarni wrote:
>> +                       if (arg1_len == NULL_TREE)
>> +                         {
>> +                           gimple_stmt_iterator gsi;
>> +                           tree strlen_decl;
>> +                           gimple *strlen_call;
>> +
>> +                           strlen_decl = builtin_decl_explicit (BUILT_IN_STRLEN);
>> +                           strlen_call = gimple_build_call (strlen_decl, 1,
>> +                                                            arg1);
>> +                           arg1_len = make_ssa_name (size_type_node);
>> +                           gimple_call_set_lhs (strlen_call, arg1_len);
>> +                           update_stmt (strlen_call);
>> +                           gsi = gsi_for_stmt (call_stmt);
>> +                           gsi_insert_before (&gsi, strlen_call, GSI_SAME_STMT);
>> +                         }
>
> Why?  If the strlen isn't readily available, do you really think it is
> always a win to replace one call with 2 calls?  The string you want to do
> strlen on can be huge, the haystack could be empty or very short, etc.
> I'd just punt if strlen isn't known.
>> +
>> +                       gimple_stmt_iterator gsi = gsi_for_stmt (call_stmt);
>> +                       tree memcmp_decl = builtin_decl_explicit (BUILT_IN_MEMCMP);
>> +                       gcall *memcmp_call
>> +                         = gimple_build_call (memcmp_decl, 3, arg0, arg1,
>> +                                              arg1_len);
>> +                       tree memcmp_lhs = make_ssa_name (integer_type_node);
>> +                       gimple_call_set_lhs (memcmp_call, memcmp_lhs);
>> +                       update_stmt (memcmp_call);
>> +                       gsi_remove (&gsi, true);
>> +                       gsi_insert_before (&gsi, memcmp_call, GSI_SAME_STMT);
>> +
>> +                       gsi = gsi_for_stmt (stmt);
>> +                       tree zero = build_zero_cst (TREE_TYPE (memcmp_lhs));
>> +                       gassign *ga = gimple_build_assign (lhs, code,
>> +                                                          memcmp_lhs, zero);
>> +                       gsi_replace (&gsi, ga, false);
>> +                       update_ssa (TODO_update_ssa);
>
> And this is certainly even more wrong than the old TODO_update_ssa at the
> end of the pass, now you'll do it for every single replacement in the
> function.  Why do you need it?  The old call stmt has gimple_vdef and
> gimple_vuse, so just copy those over, see how e.g.
> replace_call_with_call_and_fold in gimple-fold.c does that.
> If you don't add strlen, you need to move the vdef/vuse from stmt to
> memcmp_call, if you really want to add strlen (see above note though),
> then that call should have a vuse added (same vuse as the stmt originally
> had).
Hi,
Thanks for the suggestions. In attached patch, I dropped the transform
if strlen (t) is unknown.
Since strstr is marked pure, so IIUC call_stmt for strstr shouldn't
have vdef assoicated with it ?
(gimple_vdef for call_stmt returned NULL for test-cases I tried it
with). Moving gimple_vuse
from call_stmt to memcmp_call worked for me.
Does the patch look OK ?
Bootstrap+tested on x86_64-unknown-linux-gnu with --enable-langauges=all,ada
Cross-tested on arm*-*-*, aarch64*-*-*.

Thanks,
Prathamesh
>
>         Jakub

Attachment: tcwg-701-7.diff
Description: Text document