From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17076 invoked by alias); 17 Apr 2019 11:14:19 -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 17068 invoked by uid 89); 17 Apr 2019 11:14:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-7.9 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=booking X-HELO: mail-it1-f193.google.com Received: from mail-it1-f193.google.com (HELO mail-it1-f193.google.com) (209.85.166.193) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 17 Apr 2019 11:14:18 +0000 Received: by mail-it1-f193.google.com with SMTP id u65so3673427itc.2 for ; Wed, 17 Apr 2019 04:14:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+FffC9YMe0YOumhBPEPZFzrgppOURUIGkwxOnDd5eMk=; b=Csh74sBwhp2ClV7DIA5m1BienmaLNPtyudg6q3AfD9DTllh7VHcAioBEmtKSBp3+x4 mXXFeqtzEj2tEHNLXVQR+6L+IhSBfOTut0sqozhtDuDLwYcbIpZOODiAsxFj2pyBmcHx gFgtLTSjpGVz9i8tQe/jzUsR70y+GWrdMJF/bkk//XqOr3R2zPuzzuvDhyQ2SHZ7yXeG OmODDsGd8rJIYc/NqCPgbrhA24zIMa/oaiMgcTcE+r+d6AIDOGHgzJHEA62Zd3kmNwb1 zIaLqAi7o3xkRO9wfPNwhjjLq9E/ljQxwO69tqw8uhMhQIfBBgw9cLk6KDQIpPC/NXqG eAvQ== MIME-Version: 1.0 References: <7cb22a67-89e5-45d3-aef4-398311416140.bin.cheng@linux.alibaba.com> <20190417071001.GR21066@tucnak> In-Reply-To: <20190417071001.GR21066@tucnak> From: "Bin.Cheng" Date: Wed, 17 Apr 2019 11:14:00 -0000 Message-ID: Subject: Re: [PATCH PR90078]Capping comp_cost computation in ivopts To: Jakub Jelinek Cc: "bin.cheng" , GCC Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-04/txt/msg00698.txt.bz2 On Wed, Apr 17, 2019 at 3:10 PM Jakub Jelinek wrote: > > On Wed, Apr 17, 2019 at 02:13:12PM +0800, bin.cheng wrote: > > Hi, > > As discussed in PR90078, this patch checks possible infinite_cost overflow in ivopts. > > Also as discussed, overflow happens mostly because of cost scaling wrto bb_freq/loop_freq. > > For the moment, we only implement capping in comp_cost operators, while in next > > stage1, we may instead implement capping in get_scaled_computation_cost_at with > > more supporting benchmark data. > > > > BTW, I think switching costs around comparison between infinite_cost is unnecessary > > since there will be no overflow in integer after capping with infinite_cost. > > > > Bootstrap and test on x86_64, is it OK? > > > > Thanks, > > bin > > > > 2019-04-17 Bin Cheng > > > > PR tree-optimization/92078 > > * tree-ssa-loop-ivopts.c (comp_cost::operator +,-,+=,-+,/=,*=): Add > > checks for infinite_cost overflow. > > > > 2018-04-17 Bin Cheng > > > > PR tree-optimization/92078 > > * gcc/testsuite/g++.dg/tree-ssa/pr90078.C: New test. > > --- a/gcc/tree-ssa-loop-ivopts.c > +++ b/gcc/tree-ssa-loop-ivopts.c > @@ -243,6 +243,9 @@ operator+ (comp_cost cost1, comp_cost cost2) > if (cost1.infinite_cost_p () || cost2.infinite_cost_p ()) > return infinite_cost; > > + if (cost1.cost + cost2.cost >= infinite_cost.cost) > + return infinite_cost; > > As > #define INFTY 10000000 > what is the reason to keep the previous condition as well? > I mean, if cost1.cost == INFTY or cost2.cost == INFTY, > cost1.cost + cost2.cost >= INFTY too. > Unless costs can go negative. It's a bit complicated, but in general, costs can go negative. > > @@ -256,6 +259,8 @@ operator- (comp_cost cost1, comp_cost cost2) > return infinite_cost; > > gcc_assert (!cost2.infinite_cost_p ()); > + if (cost1.cost - cost2.cost >= infinite_cost.cost) > + return infinite_cost; > > Unless costs can be negative, when you first bail out > for cost1.cost == INFTY, then cost1.cost - cost2.cost won't > be INFTY (but could get negative). So shouldn't there be a guard against > that instead? Or, if costs can be negative, shouldn't there be also > guards that it doesn't grow too negative (say smaller than -INFTY)? Negative cost is kind of a result of booking cost cancellation at different place. For example, it mostly comes from in modeling auto increment/decrement addressing mode. To be specific, when IV's increment instruction can be merged into addressing mode, we cancel cost of IV increment operation in cand-use cost. Very likely 4 will be subtracted. In general, we wouldn't expect negative cost can go too big, so there is no -INFTY logic in ivopts at all. So this is the least invasive fix for the moment, I would consider capping bb_freq/loop_freq in the future which should rule out the overflow possibility in the first place. Thanks, bin > > Jakub