[PATCH, RFC, rs6000] PR80791 Consider doloop in ivopts

[Kewen.Lin]

Hi Bin,

Sorry for late response (just back from vacation).

Thanks very much for your comments.

on 2019/4/27 上午11:20, Bin.Cheng wrote:
> For such non-trivial patch, we can improve review process by splitting
> to smaller patches which can be reviewed/approved independently.

Good idea! I'll split it.

> Below are some general comments specific to IVOPTs change.

>>           for (j = 0; j < group->vuses.length (); j++)
>>             {
>> -             rewrite_use_compare (data, group->vuses[j], cand);
>> -             update_stmt (group->vuses[j]->stmt);
>> +                 rewrite_use_compare (data, group->vuses[j], cand);
>> +                 update_stmt (group->vuses[j]->stmt);
> Wrong formatting change?  Also we can run contrib/check_GNU_style.sh
> for coding style issues.
> 

Good catch!

>> +  /* Some compare iv_use is probably useless once the doloop optimization
>> +     performs.  */
>> +  if (tailor_cmp_p)
>> +    tailor_cmp_uses (data);
> Function tailor_cmp_uses sets iv_use->zero_cost_p under some
> conditions.  Once the flag is set, though the iv_use participates cost
> computation in determine_group_iv_cost_cond, the result cost is
> hard-wired to ZERO (which means cost computation for such iv_use is
> waste of time). 

Yes, it can be improved by some early check and return.
But it's still helpful to make it call with may_eliminate_iv.
gcc.dg/no-strict-overflow-6.c is one example, with may_eliminate_iv 
consideration it exposes more opportunities for downstream optimization.

> Also iv_use rewriting process is skipped for related
> ivs preserved explicitly by preserve_ivs_for_use.
> Note IVOPTs already adds candidate for original ivs.  So why not
> detecting doloop iv_use, adjust its cost with the corresponding
> original iv candidate, then let the general cost model make the
> decision?  I believe this better fits existing infrastructure and
> requires less change, for example, preserve_ivs_for_use won't be
> necessary.  
I agree adjusting the cost of original iv candidate of the iv_use
requires less change, but on one hand, I thought to remove interest
cmp iv use or make it zero cost is close to the fact.  Since it's 
eliminated eventually in doloop optimization, it should not 
considered in cost modeling.  This way looks more exact.
One the other hand, I assumed your suggestion is to increase the 
cost for the pair (original iv cand, cmp iv use), the increase cost 
seems to be a heuristic value?  It seems it's possible to sacrifice
performance for some worst cases?  Although it's integrated to the
existing framework, it probably introduces other cost adjust issues.
For example, the original iv cand is inclined not to be selected after
this change, but without considering the cmp iv use, it's better to 
be selected.  It looks highly depend on the cost tuning? 

Does my understanding above make sense? 

But I will follow your suggestion to update and check the regression 
testing result etc. I just updated the code to increase the cost for 
the pair (original iv cand, cmp iv use) with 5 (to balance the original 
iv 4 vs other 5), the failure case in PR80791 was fixed.  I assumed
IP_ORIGINAL for cand->pos to check original iv cand is enough.


> It has other advantages too, for example, 1) candidate of
> original iv can be preferred for other iv_uses with doloop cost

I think the patch way is not intrusive either.

> tuning;  2) the doloop decision can still be canceled by cost model if
> it's really not beneficial.  With current patch, it can't be undo once
> the decision is made (at very early stage in IVOPTs.).

I can't really follow this.  If it's predicted to be transformed to doloop,
I think it should not be undoed any more, since it's useless to consider 
this cmp iv use. Whatever IVOPTS does, the comp at loop closing should not
be changed (although possible to use other iv), right?  Do I miss something?

> Sorry didn't bring out this earlier, I only realized this after
> reading your code.

It doesn't matter indeed!  Thanks again!