On Sun, May 5, 2019 at 11:23 AM Kewen.Lin <li...@linux.ibm.com> wrote:
>
> 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.
Hmm, I wasn't suggesting early check and return, on the contrary, we
can better integrate doloop/cost stuff in the overall model. See more
in following comments.
>
> > 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.
Whether doloop transformation should be considered or be bypassed in
cost model isn't a problem. Actually we can bind doloop iv_cand to
cmp iv_use in order to force the transformation. My concern is the
patch specially handles doloop by setting the special flag, then
checking it. We generally avoid such special-case handling since it
hurts long time maintenance. The pass was very unstable in the pass
because of such issues.
> 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
Decrease the cost so that the iv_cand is preferred? The comment
wasn't made on top of implementing doloop in ivopts. Anyway, you can
still bypass cost model by binding the "correct" iv_cand to cmp
iv_use.
> 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?
As mentioned, the previous comment wasn't made on top of implementing
doloop in ivopts. That would be nice but a different story.
Before we can do that, it'd better be conservative and only makes
(doloop) decision in ivopts when you are sure. As you mentioned, it's
hard to do the same checks at gimple as RTL, right? In this case,
making it a (conservative) heuristic capturing certain beneficial
cases sounds better than capturing all cases but fail in later RTL
passes.
Thanks,
bin
>
> > Sorry didn't bring out this earlier, I only realized this after
> > reading your code.
>
> It doesn't matter indeed! Thanks again!
>
