Re: [PATCH v4] vect/rs6000: Support vector with length cost modeling

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Jul 31, 2020 at 1:03 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
> >>> +      bool niters_known_p = LOOP_VINFO_NITERS_KNOWN_P (loop_vinfo);
> >>> +      bool need_iterate_p
> >>> +   = (!LOOP_VINFO_EPILOGUE_P (loop_vinfo)
> >>> +      && !vect_known_niters_smaller_than_vf (loop_vinfo));
> >>> +
> >>> +      /* Init min/max, shift and minus cost relative to single
> >>> +    scalar_stmt. For now we only use length-based partial vectors on
> >>> +    Power, target specific cost tweaking may be needed for other
> >>> +    ports in future.  */
> >>> +      unsigned int min_max_cost = 2;
> >>> +      unsigned int shift_cost = 1, minus_cost = 1;
> >>
> >> Please instead add a scalar_min_max to vect_cost_for_stmt, and use
> >> scalar_stmt for shift and minus.  There shouldn't be any Power things
> >> hard-coded into target-independent code.
> >>
> >
> > Agree!  It's not good to leave them there.  I thought to wait and see
> > if other targets which support vector with length can reuse this, or
> > move it to target specific codes then if not sharable.  But anyway
> > it looks not good, let's fix it.

In other generic places like this we simply use three generic scalar_stmt
costs.  At least I don't see how min_max should be different from it
when shift can be the same as minus.  Note this is also how we treat
vectorization of MAX_EXPR - scalar cost is one scalar_stmt and
vector cost is one vector_stmt.  As you say below the add_stmt_cost
hook can adjust based on the actual GIMPLE stmt -- if one is
available (which indeed it isn't here).

I'm somewhat lacking context here as well - we actually GIMPLE
code-generate the min/max/shift/minus and only the eventual
AND is defered to the target, right?

> > I had some concerns on vect_cost_for_stmt way, since it seems to allow
> > more computations similar to min/max to be added like this, in long
> > term it probably leads to the situtation like: scalar_min_max,
> > scalar_div_expr, scalar_mul_expr ... an enum (cost types) bloat, it
> > seems not good to maintain.
>
> I guess doing that doesn't seem so bad to me :-)  I think it's been
> a recurring problem that the current classification isn't fine-grained
> enough for some cases.

But we eventually want to get rid of this classification enum in favor
of the add_stmt_cost hook ...

> > I noticed that i386 port ix86_add_stmt_cost will check stmt_info->stmt,
> > whether is assignment and the subcode of the expression, it provides the
> > chance to check the statement more fine-grain, not just as normal
> > scalar_stmt/vector_stmt.
> >
> > For the case here, we don't have the stmt_info, but we know the type
> > of computation(expression), how about to extend the hook add_stmt_cost
> > with one extra tree_code type argument, by default it can be some
> > unmeaningful code, for some needs like here, we specify the tree_code
> > as the code of computation, like {MIN,MAX}_EXPR, then target specific
> > add_stmt_cost can check this tree_code and adjust the cost accordingly.
>
> If we do that, I guess we should “promote” code_helper out of
> gimple-match.h and use that instead, so that we can handle
> internal and built-in functions too.
>
> Would like to hear Richard's opinion on the best way forward here.

I'd say defer this to a later patch and for now simply cost one scalar
stmt for MIN/MAX.  I agree that if we add a tree_code we want a
code_helper instead.  Note that I want to eventually have a
full SLP tree for the final code generation where all info should be
there (including SLP nodes for those min/max ops) and which the
target could traverse.  But I'm not sure if I can make enough progress
on that SLP-only thing for GCC 11 even...

Richard.

> Thanks,
> Richard