From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15193 invoked by alias); 25 May 2010 18:12:37 -0000 Received: (qmail 15184 invoked by uid 22791); 25 May 2010 18:12:37 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from nikam-dmz.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 25 May 2010 18:12:32 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 29025) id E8AB99AC906; Tue, 25 May 2010 20:12:29 +0200 (CEST) Date: Tue, 25 May 2010 18:25:00 -0000 From: Zdenek Dvorak To: Xinliang David Li Cc: GCC Patches Subject: Re: IVOPT improvement patch Message-ID: <20100525181229.GA2161@kam.mff.cuni.cz> References: <20100511071800.GA11044@kam.mff.cuni.cz> <20100525093244.GA24987@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-2 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.18 (2008-05-17) 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 X-SW-Source: 2010-05/txt/msg01905.txt.bz2 Hi, > >> @@ -3811,6 +3841,9 @@ get_computation_cost_at (struct ivopts_d > >>                                        &offset, depends_on)); > >>      } > >> > >> +  /* Loop invariant computation.  */ > >> +  cost.cost /= avg_loop_niter (data->current_loop); > >> + > > > > This is wrong, at least some parts of the computation here are not loop invariant. > > Which part is not loop invariant? it depends on the actual form of the use. But in the most general case, the computation whose cost is determined here is ubase + ratio * (var - cbase), and no part of this is loop invariant (except for the force_var_costs of ubase and cbase). > >> @@ -4056,20 +4090,16 @@ may_eliminate_iv (struct ivopts_data *da > >>    /* If not, and if this is the only possible exit of the loop, see whether > >>       we can get a conservative estimate on the number of iterations of the > >>       entire loop and compare against that instead.  */ > >> -  else if (loop_only_exit_p (loop, exit)) > >> +  else > > > > This change is wrong, the test is necessary.  See > > http://gcc.gnu.org/ml/gcc-patches/2008-07/msg00146.html > > and the following discussion. > > > > The original fix to the problem is too conservative -- if there is > only one exit has the test to be replaced, it should be ok to do it, > right? Yes. But I do not see your point -- your patch removes the loop_only_exit_p test, which is necessary. > >>      { > >>        double_int period_value, max_niter; > >>        if (!estimated_loop_iterations (loop, true, &max_niter)) > >>       return false; > >>        period_value = tree_to_double_int (period); > >> -      if (double_int_ucmp (max_niter, period_value) >= 0) > >> +      if (double_int_ucmp (max_niter, period_value) > 0) > >>       return false; > >>      } > > > > This also seems wrong (or at least inconsistent with that is done for > > the constant number of iterations). > > This looks correct to me. I think you are right; but, then the preceding test for tree_int_cst_lt should be changed as well (so that both conditions are the same). It would also be nice to add testcases for the boundary values to the testsuite, to make sure we are not making an off-by-one error. Zdenek