[PATCH] rs6000: Make some BIFs vectorized on P10

Kewen.Lin linkw@linux.ibm.com
Wed Aug 25 05:17:14 GMT 2021

on 2021/8/25 上午6:14, Segher Boessenkool wrote:
> Hi!
> On Fri, Aug 13, 2021 at 10:34:46AM +0800, Kewen.Lin wrote:
>> on 2021/8/12 下午11:10, Segher Boessenkool wrote:
>>>> +      && VECTOR_UNIT_ALTIVEC_OR_VSX_P (in_vmode))
>>>> +    {
>>>> +      machine_mode exp_mode = DImode;
>>>> +      machine_mode exp_vmode = V2DImode;
>>>> +      enum rs6000_builtins vname = RS6000_BUILTIN_COUNT;
>>> "name"?  This should be "bif" or similar?
>> Updated with name.
> No, I meant "name" has no meaning other than it is wrong here :-)
> It is an enum for which builtin to use here.  It has nothing to do with
> a name.  So it could be "enum rs6000_builtins bif" or whatever you want;
> short variable names are *good*, for many reasons, but they should not
> egregiously lie :-)

Oops, sorry for the misunderstanding, will update it with "bif".

>>>> +/* { dg-do run } */
>>>> +/* { dg-require-effective-target lp64 } */
>>> Same here.  I suppose this uses builtins that do not exist on 32-bit?
>> Yeah, those bifs which are guarded with lp64 in their cases are only
>> supported on 64-bit environment.
> It is a pity we cannot use "powerpc64" here (that selector does not test
> what you would/could/should hope it tests...  Maybe someone can fix it
> some day?  The only real blocker to that is fixing up the current users
> of it, the rest is easy).

If I got it right, there is only one test case using this selector:


The selector checking looks interesting to me, it has special option with
"-mcpu=G5" and seems to exclude all "aix" (didn't verify it yet).

I guess there still would be some efforts to re-direct those existing
cases which should use new "powerpc64_ok" instead of "lp64"?

> Expanding a bit...  You would expect (well, I do!  I did patches
> expecting this several times) this to mean "powerpc64_ok", but it in
> fact means "powerpc64_hw".  Maybe we should have selectors with those
> two names, and get rid of the current "powerpc64"?

Yeah, it sounds good to have those two names just like some existing.

>>>> +#define CHECK(name)                                                           \
>>>> +  __attribute__ ((optimize (1))) void check_##name ()                         \
>>> What is the attribute for, btw?  It seems fragile, but perhaps I do not
>>> understand the intention.
>> It's to stop compiler from optimizing check functions with vectorization,
>> since the test point is to compare the results between scalar and vectorized
>> version.
> So, add a comment for this as well please.
> In general, in testcases you can do the dirtiest things, no problems at
> all, just document what you do why :-)

OK, will add.

>> Thanks, v2 has been attached by addressing Bill's and your comments.  :)
> Looks good.  Just fix that "name" thing, and it is okay for trunk.
> Thanks!

Thanks for the review!


More information about the Gcc-patches mailing list