From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 128797 invoked by alias); 23 Jul 2015 10:42:11 -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 128787 invoked by uid 89); 23 Jul 2015 10:42:11 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=BAYES_00,FREEMAIL_FROM,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: fencepost.gnu.org Received: from fencepost.gnu.org (HELO fencepost.gnu.org) (208.118.235.10) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 23 Jul 2015 10:42:09 +0000 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43587) by fencepost.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1ZIDx0-0002Kd-S9 for gcc-patches@gnu.org; Thu, 23 Jul 2015 06:42:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZIDwz-0005t8-Da for gcc-patches@gnu.org; Thu, 23 Jul 2015 06:42:06 -0400 Received: from mail-ig0-x22c.google.com ([2607:f8b0:4001:c05::22c]:33144) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIDwz-0005t0-92 for gcc-patches@gnu.org; Thu, 23 Jul 2015 06:42:05 -0400 Received: by igbpg9 with SMTP id pg9so10561190igb.0 for ; Thu, 23 Jul 2015 03:42:04 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.50.72.113 with SMTP id c17mr42823716igv.73.1437648124753; Thu, 23 Jul 2015 03:42:04 -0700 (PDT) Received: by 10.107.142.7 with HTTP; Thu, 23 Jul 2015 03:42:04 -0700 (PDT) In-Reply-To: <55AFBE25.5020508@mentor.com> References: <55A6C1DF.1050108@mentor.com> <20150720183141.GB20717@f1.c.bardezibar.internal> <55AD9093.1060206@mentor.com> <55AE5340.2010700@mentor.com> <20150721184249.GA7417@f1.c.bardezibar.internal> <55AFBE25.5020508@mentor.com> Date: Thu, 23 Jul 2015 10:51:00 -0000 Message-ID: Subject: Re: [PATCH] Don't allow unsafe reductions in graphite From: Richard Biener To: Tom de Vries Cc: Sebastian Pop , "gcc-patches@gnu.org" Content-Type: text/plain; charset=UTF-8 X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2607:f8b0:4001:c05::22c X-IsSubscribed: yes X-SW-Source: 2015-07/txt/msg01917.txt.bz2 On Wed, Jul 22, 2015 at 6:00 PM, Tom de Vries wrote: > [ was: Re: [RFC, PR66873] Use graphite for parloops ] > > On 22/07/15 13:02, Richard Biener wrote: >> >> On Wed, Jul 22, 2015 at 1:01 PM, Richard Biener >> wrote: >>> >>> >On Tue, Jul 21, 2015 at 8:42 PM, Sebastian Pop wrote: >>>> >>>> >>Tom de Vries wrote: >>>>> >>>>> >>>Fix reduction safety checks >>>>> >>> >>>>> >>> * graphite-sese-to-poly.c (is_reduction_operation_p): Limit >>>>> >>> flag_associative_math to SCALAR_FLOAT_TYPE_P. Honour >>>>> >>> TYPE_OVERFLOW_TRAPS and TYPE_OVERFLOW_WRAPS for >>>>> >>> INTEGRAL_TYPE_P. >>>>> >>> Only allow wrapping fixed-point otherwise. >>>>> >>> (build_poly_scop): Always call >>>>> >>> rewrite_commutative_reductions_out_of_ssa. >>>> >>>> >> >>>> >>The changes to graphite look good to me. >>> >>> > >>> >+ if (SCALAR_FLOAT_TYPE_P (type)) >>> >+ return flag_associative_math; >>> >+ >>> > >>> >why only scalar floats? > > > Copied from the conditions in vect_is_simple_reduction_1. > >>> >Please use FLOAT_TYPE_P. > > Done. > >>> > >>> >+ if (INTEGRAL_TYPE_P (type)) >>> >+ return (!TYPE_OVERFLOW_TRAPS (type) >>> >+ && TYPE_OVERFLOW_WRAPS (type)); >>> > >>> >it cannot both wrap and trap thus TYPE_OVERFLOW_WRAPS is enough. >>> > > > > Done. > >>> >I'm sure you'll disable quite some parallelization this way... (the >>> >routine is modeled after >>> >the vectorizers IIRC, so it would be affected as well). Yeah - I see >>> >you modify autopar >>> >testcases. > > > I now split up the patch, this bit only relates to graphite, so no autopar > testcases are affected. > >>> >Please instead XFAIL the existing ones and add variants >>> >with unsigned >>> >reductions. Adding -fwrapv isn't a good solution either. > > > Done. > >>> > >>> >Can you think of a testcase that breaks btw? >>> > > > > If you mean a testcase that fails to execute properly with the fix, and > executes correctly with the fix, then no. The problem this patch is trying > to fix, is that we assume wrapping overflow without fwrapv. In order to run > into a runtime failure, we need a target that does not do wrapping overflow > without fwrapv. > >>> >The "proper" solution (see other passes) is to rewrite the reduction >>> >to a wrapping >>> >one (cast to unsigned for the reduction op). >>> > > > > Right. > >>> >+ return (FIXED_POINT_TYPE_P (type) >>> >+ && FIXED_POINT_TYPE_OVERFLOW_WRAPS_P (type)); >>> > >>> >why? > > > Again, copied from the conditions in vect_is_simple_reduction_1. > >>> > Simply return false here instead? > > Done. > > > [ Btw, looking at associative_tree_code, I realized that the > overflow checking is only necessary for PLUS_EXPR and MULT_EXPR: > ... > switch (code) > { > case BIT_IOR_EXPR: > case BIT_AND_EXPR: > case BIT_XOR_EXPR: > case PLUS_EXPR: > case MULT_EXPR: > case MIN_EXPR: > case MAX_EXPR: > return true; > ... > > The other operators cannot overflow to begin with. My guess is that it's > better to leave this for a trunk-only follow-up patch. > ] > > Currently bootstrapping and reg-testing on x86_64. > > OK for trunk? > > OK 5 and 4.9 release branches? Ok if Sebastian is fine with it. Richard. > Thanks, > - Tom >