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

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Richards,

on 2020/7/31 下午7:20, Richard Biener wrote:
> 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

Yeah, normally they (min/max/minus/shift) are taken as scalar_stmt, excepting
for fine-grain tuning like i386 port, they will use the same cost.  On Power9,
to implement min/max it takes double cycles of the normal scalar operations
like add/shift, I was trying to model it more fine-grained since we probably
generate a few min/max here, if the loop body cost is small, I was worried
the decision isn't good enough.  But yeah, in other generic places, the small
loop could also suffer this similar off, they are the same essentially.

> 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?
> 

Yes, min/max/shift/minus are all GIMPLE code, targets like Power
will have its target specific cost for shift which moves length
to high bits 0:7.

One typical case is as below:

  <bb 3> [local count: 105119324]:
  _26 = n_11(D) * 4;
  _37 = MAX_EXPR <_26, 16>;
  _38 = _37 + 18446744073709551600;
  _40 = MIN_EXPR <_26, 16>;

  <bb 4> [local count: 630715945]:
  # ivtmp_35 = PHI <0(3), ivtmp_36(4)>
  # loop_len_30 = PHI <_40(3), _44(4)>
  _19 = &MEM[base: a_12(D), index: ivtmp_35, offset: 0B];
  vect_24 = .LEN_LOAD (_19, 4B, loop_len_30);
  vect__3.7_23 = VIEW_CONVERT_EXPR<vector(4) unsigned int>(vect_24);
  _1 = &MEM[base: b_13(D), index: ivtmp_35, offset: 0B];
  vect_17 = .LEN_LOAD (_1, 4B, loop_len_30);
  vect__5.10_9 = VIEW_CONVERT_EXPR<vector(4) unsigned int>(vect_17);
  vect__7.11_8 = vect__5.10_9 + vect__3.7_23;
  vect_28 = VIEW_CONVERT_EXPR<vector(16) unsigned char>(vect__7.11_8);
  _2 = &MEM[base: c_14(D), index: ivtmp_35, offset: 0B];
  .LEN_STORE (_2, 4B, loop_len_30, vect_28);
  _42 = MIN_EXPR <ivtmp_35, _38>;
  _43 = _38 - _42;
  _44 = MIN_EXPR <_43, 16>;
  ivtmp_36 = ivtmp_35 + 16;
  if (ivtmp_35 < _38)
    goto <bb 4>; [83.33%]
  else
    goto <bb 5>; [16.67%]


>>> 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 ...
> 

Nice, sounds like each target has to handle it fine-grain.  :) 
IIUC, the current modeling doesn't consider the instruction dependency and
execution resource etc. like scheduling, even if all costs are fine-grained
enough, the decision could be sub-optimal.

>>> 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...
> 

OK, I'm fine to take MIN/MAX as scalar_stmt here.  Thank both of you!
This new SLP framework looks very promising and powerful.  :-)


BR,
Kewen