RE: [Ping]Two pending IVOPT patches

["bin.cheng" <bin dot cheng at arm dot com>]

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Jeff Law
> Sent: Tuesday, December 10, 2013 6:31 AM
> To: Bin.Cheng
> Cc: gcc-patches List; Richard Biener; Zdenek Dvorak
> Subject: Re: [Ping]Two pending IVOPT patches
> 
> On 11/26/13 03:52, Bin.Cheng wrote:
> > On Tue, Nov 26, 2013 at 6:06 AM, Jeff Law <law@redhat.com> wrote:
> >> On 11/25/13 02:11, Bin.Cheng wrote:
> >>>
> >>>
> >>> Slightly tune to make iv cand choosing algorithm more accurate:
> >>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg01574.html
> >>
> >> It would help if you had some sample codes where this patch was
> >> useful.  I can kind-of see what's going on, but I'm way too
> >> unfamiliar with the tree-IV code to review this change.
> >>
> > Jeff, Thanks to your review.
> > As for example, consider below code on ARM/cortex-m4, (the code itself
> > may be non-sense):
> [ snip ]
> 
> So in iv_ca_narrow you have the following change:
> @@ -5705,13 +5709,15 @@ iv_ca_narrow (struct ivopts_data *data, struct
> iv_
>   	      if (!cp)
>   		continue;
> 
> -	      if (!iv_ca_has_deps (ivs, cp))
> -                continue;
> +	      iv_ca_set_cp (data, ivs, use, cp);
> +	      acost = iv_ca_cost (ivs);
> +	      iv_ca_set_cp (data, ivs, use, old_cp);
> 
> -	      if (!cheaper_cost_pair (cp, new_cp))
> -		continue;
> -
> -	      new_cp = cp;
> +	      if (compare_costs (acost, best_cost) < 0)
> +		{
> +		  best_cost = acost;
> +		  new_cp = cp;
> +		}
>   	    }
>   	}
>         else
> 
> You're set a new cost pair (cp), compute the cost into acost, the restore
the
> old cost pair (oldcp).  Then you see if you got a cheaper cost and if so,
record
> the best cost and set new_cp.
> 
> What's the point behind restoring the old cost pair within the loop?
> Can't you just restore it outside the loop if nothing cheaper was found?

Yes, I updated the patch as attached.  I also skipped the computation for
START in the inner most loop.  Actually there is another chance to reduce
computation by merging the restore of old cost pair and the first call of
iv_ca_delta_commit if we don't skip computation for START, but that should
be done in another patch.

> 
> AFAICT, the calls to iv_ca_has_deps aren't there for correctness, they
were
> there to prune out IVs that were not useful in this loop, correct?
Emm, some kind of.  See the cost of iv candidate set consists of several
parts, the representation cost in cost pair; the register pressure cost
falls in dependence on invariant expressions, etc..  Here iv_ca_has_deps
checks whether new cost pair depends on other invariant expression which is
not involved before, if so, current algorithm considers the new cost pair
has higher cost and just skips.  In fact, the new cost pair may give us
lower overall cost even it introduces new dependence(similar to the case I
gave).  That's why I used the overall cost comparison for good.

Is this new version patch looks good to you? I will re-test if it's ok.

Thanks,
bin
> 
> Jeff

Attachment: ivopt-improve_iv_set-20131210.txt
Description: Text document