From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23216 invoked by alias); 18 Apr 2014 11:24:58 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 23205 invoked by uid 89); 18 Apr 2014 11:24:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_50,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 18 Apr 2014 11:24:56 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s3IBOrt5030615 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 18 Apr 2014 07:24:54 -0400 Received: from tucnak.zalov.cz (ovpn-116-17.ams2.redhat.com [10.36.116.17]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id s3IBOpOp012814 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 18 Apr 2014 07:24:53 -0400 Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.14.8/8.14.7) with ESMTP id s3IBOnxG020082; Fri, 18 Apr 2014 13:24:49 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.14.8/8.14.8/Submit) id s3IBOlOY020081; Fri, 18 Apr 2014 13:24:47 +0200 Date: Fri, 18 Apr 2014 11:31:00 -0000 From: Jakub Jelinek To: Marc Glisse Cc: Richard Biener , GCC Patches Subject: Re: calloc = malloc + memset Message-ID: <20140418112447.GB1817@tucnak.redhat.com> Reply-To: Jakub Jelinek References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2014-04/txt/msg01081.txt.bz2 On Sun, Mar 23, 2014 at 05:13:46PM +0100, Marc Glisse wrote: > 2014-03-23 Marc Glisse > > PR tree-optimization/57742 > gcc/ > * tree-ssa-strlen.c (get_string_length): Ignore malloc. > (handle_builtin_malloc, handle_builtin_memset): New functions. > (strlen_optimize_stmt): Call them. > * passes.def: Move strlen after loop+dom. > gcc/testsuite/ > * g++.dg/tree-ssa/calloc.C: New testcase. > * gcc.dg/tree-ssa/calloc-1.c: Likewise. > * gcc.dg/tree-ssa/calloc-2.c: Likewise. > * gcc.dg/strlenopt-9.c: Adapt. Some CL lines are indented by 8 spaces instead of tabs. The passes.def change makes me a little bit nervous, but if it works, perhaps. > --- gcc/testsuite/g++.dg/tree-ssa/calloc.C (revision 0) > +++ gcc/testsuite/g++.dg/tree-ssa/calloc.C (working copy) > @@ -0,0 +1,35 @@ > +/* { dg-do compile { target c++11 } } */ > +/* { dg-options "-O3 -fdump-tree-optimized" } */ > + > +#include > +#include > +#include > + > +void g(void*); > +inline void* operator new(std::size_t sz) > +{ > + void *p; > + > + if (sz == 0) > + sz = 1; > + > + // Slightly modified from the libsupc++ version, that one has 2 calls > + // to malloc which makes it too hard to optimize. > + while ((p = std::malloc (sz)) == 0) > + { > + std::new_handler handler = std::get_new_handler (); > + if (! handler) > + throw std::bad_alloc(); > + handler (); > + } > + return p; > +} > + > +void f(void*p,int n){ > + new(p)std::vector(n); > +} > + > +/* { dg-final { scan-tree-dump-times "calloc" 1 "optimized" } } */ > +/* { dg-final { scan-tree-dump-not "malloc" "optimized" } } */ > +/* { dg-final { scan-tree-dump-not "memset" "optimized" } } */ > +/* { dg-final { cleanup-tree-dump "optimized" } } */ This looks to me way too much fragile, any time the libstdc++ or glibc headers change a little bit, you might need to adjust the dg-final directives. Much better would be if you just provided the prototypes yourself and subset of the std::vector you really need for the testcase. You can throw some class or int, it doesn't have to be std::bad_alloc, etc. > --- gcc/testsuite/gcc.dg/strlenopt-9.c (revision 208772) > +++ gcc/testsuite/gcc.dg/strlenopt-9.c (working copy) > @@ -11,21 +11,21 @@ fn1 (int r) > optimized away. */ > return strchr (p, '\0'); > } > > __attribute__((noinline, noclone)) size_t > fn2 (int r) > { > char *p, q[10]; > strcpy (q, "abc"); > p = r ? "a" : q; > - /* String length for p varies, therefore strlen below isn't > + /* String length is constant for both alternatives, and strlen is > optimized away. */ > return strlen (p); Is this because of jump threading? > --- gcc/testsuite/gcc.dg/tree-ssa/calloc-1.c (revision 0) > +++ gcc/testsuite/gcc.dg/tree-ssa/calloc-1.c (working copy) > @@ -0,0 +1,29 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-tree-optimized" } */ > + > +#include > +#include Even this I find unsafe. The strlenopt*.c tests use it's custom strlenopt.h header for a reason, you might just add a calloc prototype in there and use that header. > --- gcc/tree-ssa-strlen.c (revision 208772) > +++ gcc/tree-ssa-strlen.c (working copy) > @@ -496,20 +496,23 @@ get_string_length (strinfo si) > case BUILT_IN_STPCPY_CHK: > gcc_assert (lhs != NULL_TREE); > loc = gimple_location (stmt); > si->endptr = lhs; > si->stmt = NULL; > lhs = fold_convert_loc (loc, size_type_node, lhs); > si->length = fold_convert_loc (loc, size_type_node, si->ptr); > si->length = fold_build2_loc (loc, MINUS_EXPR, size_type_node, > lhs, si->length); > break; > + case BUILT_IN_MALLOC: > + break; > + /* calloc always has si->length set. */ I guess it would be better to talk about BUILT_IN_CALLOC here, and align the comment with case and default column, to make it clear the comment is not about BUILT_IN_MALLOC. Or is it? But, see below. > default: > if (!si->dont_invalidate) > { > ao_ref r; > + // Do not use si->length. As the whole file uses /* ... */ comments, can you use them here too? > ao_ref_init_from_ptr_and_size (&r, si->ptr, NULL_TREE); > if (stmt_may_clobber_ref_p_1 (stmt, &r)) > { > set_strinfo (i, NULL); > free_strinfo (si); > continue; > } > } > si->dont_invalidate = false; > nonempty = true; > @@ -1608,20 +1612,90 @@ handle_builtin_strcat (enum built_in_fun > { > laststmt.stmt = stmt; > laststmt.len = srclen; > laststmt.stridx = dsi->idx; > } > } > else if (dump_file && (dump_flags & TDF_DETAILS) != 0) > fprintf (dump_file, "not possible.\n"); > } > > +/* Handle a call to malloc or calloc. */ > + > +static void > +handle_builtin_malloc (enum built_in_function bcode, gimple_stmt_iterator *gsi) > +{ > + gimple stmt = gsi_stmt (*gsi); > + tree lhs = gimple_call_lhs (stmt); > + gcc_assert (get_stridx (lhs) == 0); > + int idx = new_stridx (lhs); > + tree length = NULL_TREE; > + if (bcode == BUILT_IN_CALLOC) > + length = build_int_cst (size_type_node, 0); Is this safe? I mean, if you call int a = 0; ptr = calloc (a, n); or ptr = calloc (n, a); or ptr = calloc (0, 0); etc., then there is no zero termination. Sure, calling strlen or strchr etc. on the result is undefined behavior, just wondering if it isn't going to break anything. Though, the result is zero length and thus any dereferencing would be a bug, so perhaps we are fine. > +static bool > +handle_builtin_memset (gimple_stmt_iterator *gsi) > +{ > + gimple stmt1, stmt2 = gsi_stmt (*gsi); > + if (!integer_zerop (gimple_call_arg (stmt2, 1))) > + return true; > + tree ptr = gimple_call_arg (stmt2, 0); > + int idx1 = get_stridx (ptr); > + if (idx1 <= 0) > + return true; > + strinfo si1 = get_strinfo (idx1); > + if (!si1 || !(stmt1 = si1->stmt) || !is_gimple_call (stmt1)) > + return true; Please avoid assignments in if conditions if easily possible, in this case just using separate if (si1 == NULL) return true; and stmt1 = si1->stmt; and another if is IMHO better. Jakub