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:
gcc/testsuite/gcc.target/powerpc/darwin-longlong.c
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!
BR,
Kewen
