Re: [PATCH GCC8][17/33]Treat complex cand step as invriant expression

["Richard Biener via gcc-patches" <gcc-patches at gcc dot gnu dot org>]

On Thu, May 4, 2017 at 7:55 PM, Bin.Cheng <amker.cheng@gmail.com> wrote:
> On Wed, May 3, 2017 at 2:43 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Apr 18, 2017 at 12:46 PM, Bin Cheng <Bin.Cheng@arm.com> wrote:
>>> Hi,
>>> We generally need to compute cand step in loop preheader and use it in loop body.
>>> Unless it's an SSA_NAME of constant integer, an invariant expression is needed.
>>
>> I'm confused as to what this patch does.  Comments talk about "Handle step as"
>> but then we print "Depend on inv...".  And we share bitmaps, well it seems
>>
>> +         find_inv_vars (data, &step, &cand->inv_vars);
>> +
>> +         iv_inv_expr_ent *inv_expr = get_loop_invariant_expr (data, step);
>> +         /* Share bitmap between inv_vars and inv_exprs for cand.  */
>> +         if (inv_expr != NULL)
>> +           {
>> +             cand->inv_exprs = cand->inv_vars;
>> +             cand->inv_vars = NULL;
>> +             if (cand->inv_exprs)
>> +               bitmap_clear (cand->inv_exprs);
>> +             else
>> +               cand->inv_exprs = BITMAP_ALLOC (NULL);
>> +
>> +             bitmap_set_bit (cand->inv_exprs, inv_expr->id);
>>
>> just shares the bitmap allocation (and leaks cand->inv_exprs?).
>>
>> Note that generally it might be cheaper to use bitmap_head instead of
>> bitmap in the various structures (and then bitmap_initialize ()), this
>> saves one indirection.
>>
>> Anyway, the relation between inv_vars and inv_exprs is what confuses me.
>> Maybe it's the same as for cost_pair?  invariants vs. loop invariants?
>> whatever that means...
>>
>> That is, can you expand the comments in cost_pair / iv_cand for inv_vars
>> vs. inv_exprs, esp what "invariant" actually means?
> When we represent use with cand, there will be computation which is
> loop invariant.  The invariant computation is an newly created
> invariant expression and is based on ssa_vars existed before ivopts.
> If the invariant expression is complicated, we handle and call it as
> invariant expression.  We say the cost_pair depends on the inv.exprs.
> If the invariant expression is simple enough, we record all existing
> ssa_vars it based on in inv_vars.  We say the cost_pair depends on the
> inv.vars.   The same words stand for struct iv_cand.  If cand.step is
> simple enough, we simply record the ssa_var it based on in inv_vars,
> otherwise, the step is a new invariant expression which doesn't exist
> before, we record it in cand.inv_exprs.
>
> Add comment for inv_vars/inv_exprs, is this OK?   I noticed there is a
> redundant field cost_pair.inv_expr, I deleted it as obvious in a
> standalone patch.

Thanks for the explanation.

Ok.

Thanks,
Richard.

> Thanks,
> bin
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> bin
>>>
>>> 2017-04-11  Bin Cheng  <bin.cheng@arm.com>
>>>
>>>         * tree-ssa-loop-ivopts.c (struct iv_cand): New field inv_exprs.
>>>         (dump_cand): Support iv_cand.inv_exprs.
>>>         (add_candidate_1): Record invariant exprs in iv_cand.inv_exprs
>>>         for candidates.
>>>         (iv_ca_set_no_cp, iv_ca_set_cp, free_loop_data): Support
>>>         iv_cand.inv_exprs.