From: Richard Biener <richard.guenther@gmail.com>
To: "Kewen.Lin" <linkw@linux.ibm.com>
Cc: Richard Sandiford <richard.sandiford@arm.com>,
GCC Patches <gcc-patches@gcc.gnu.org>,
Bill Schmidt <wschmidt@linux.ibm.com>,
Segher Boessenkool <segher@kernel.crashing.org>
Subject: Re: [PATCH v4] vect/rs6000: Support vector with length cost modeling
Date: Fri, 31 Jul 2020 15:01:26 +0200 [thread overview]
Message-ID: <CAFiYyc2X1gKNTvOctaMQ5M0bhC3xqRXfPKBMNUqdtbrmL1MPaA@mail.gmail.com> (raw)
In-Reply-To: <4c7c935b-8eaa-1fd7-4357-72fd3239e710@linux.ibm.com>
On Fri, Jul 31, 2020 at 2:37 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> 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.
That's what the finish_cost hook is for - the target can collect
all stmts during add_stmt and then apply adjustments at the end
(IIRC power does already for shift operation resource constraints).
> >>> 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. :-)
As do all things that are still on paper^Win my head only ;)
Richard.
>
> BR,
> Kewen
next prev parent reply other threads:[~2020-07-31 13:01 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-21 5:51 [PATCH] vect: " Kewen.Lin
2020-07-21 7:57 ` Richard Biener
2020-07-22 1:26 ` [PATCH v2] vect/rs6000: " Kewen.Lin
2020-07-22 6:38 ` Richard Biener
2020-07-22 7:08 ` Kewen.Lin
2020-07-22 9:11 ` Richard Sandiford
2020-07-22 15:48 ` [PATCH v3] " Kewen.Lin
2020-07-22 16:25 ` Kewen.Lin
2020-07-24 16:21 ` Richard Sandiford
2020-07-27 3:58 ` [PATCH v4] " Kewen.Lin
2020-07-27 13:40 ` Richard Sandiford
2020-07-28 8:36 ` Kewen.Lin
2020-07-31 11:03 ` Richard Sandiford
2020-07-31 11:20 ` Richard Biener
2020-07-31 12:37 ` Kewen.Lin
2020-07-31 13:01 ` Richard Biener [this message]
2020-07-31 13:21 ` Kewen.Lin
2020-07-31 14:51 ` [PATCH v5] " Kewen.Lin
2020-08-05 7:27 ` Richard Sandiford
2020-08-05 14:06 ` Segher Boessenkool
2020-08-06 6:47 ` Kewen.Lin
2020-07-22 17:49 ` [PATCH v2] " Segher Boessenkool
2020-07-27 3:44 ` Kewen.Lin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc2X1gKNTvOctaMQ5M0bhC3xqRXfPKBMNUqdtbrmL1MPaA@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=linkw@linux.ibm.com \
--cc=richard.sandiford@arm.com \
--cc=segher@kernel.crashing.org \
--cc=wschmidt@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).