[PATCH] rs6000: Make some BIFs vectorized on P10

Bill Schmidt wschmidt@linux.ibm.com
Tue Aug 24 19:42:08 GMT 2021


Hi Kewen,

Sorry this sat in my queue for so long.  It looks like you addressed all 
of our concerns, so LGTM -- recommend maintainers approve.

Thanks!
Bill

On 8/12/21 9:34 PM, Kewen.Lin wrote:
> Hi Segher,
>
> Thanks for the review!
>
> on 2021/8/12 下午11:10, Segher Boessenkool wrote:
>> Hi!
>>
>> On Wed, Aug 11, 2021 at 02:56:11PM +0800, Kewen.Lin wrote:
>>> 	* config/rs6000/rs6000.c (rs6000_builtin_md_vectorized_function): Add
>>> 	support for some built-in functions vectorized on Power10.
>> Say which, not "some" please?
>>
> Done.
>
>>> +  machine_mode in_vmode = TYPE_MODE (type_in);
>>> +  machine_mode out_vmode = TYPE_MODE (type_out);
>>> +
>>> +  /* Power10 supported vectorized built-in functions.  */
>>> +  if (TARGET_POWER10
>>> +      && in_vmode == out_vmode
>>> +      && 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.
>
>>> +      switch (fn)
>>> +	{
>>> +	case MISC_BUILTIN_DIVWE:
>>> +	case MISC_BUILTIN_DIVWEU:
>>> +	  exp_mode = SImode;
>>> +	  exp_vmode = V4SImode;
>>> +	  if (fn == MISC_BUILTIN_DIVWE)
>>> +	    vname = P10V_BUILTIN_DIVES_V4SI;
>>> +	  else
>>> +	    vname = P10V_BUILTIN_DIVEU_V4SI;
>>> +	  break;
>>> +	case MISC_BUILTIN_DIVDE:
>>> +	case MISC_BUILTIN_DIVDEU:
>>> +	  if (fn == MISC_BUILTIN_DIVDE)
>>> +	    vname = P10V_BUILTIN_DIVES_V2DI;
>>> +	  else
>>> +	    vname = P10V_BUILTIN_DIVEU_V2DI;
>>> +	  break;
>> All of the above should not be builtin functions really, they are all
>> simple arithmetic :-(  They should not be UNSPECs either, on RTL level.
>> They can and should be optimised in real code as well.  Oh well.
>>
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/powerpc/dive-vectorize-2.c
>>> @@ -0,0 +1,12 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-require-effective-target lp64 } */
>> Please add a comment what this is needed for?  "We scan for dive*d" is
>> enough, but without anything, it takes time to figure this out.
>>
> Done, same for below requests on lp64 commentary.
>
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/powerpc/dive-vectorize-run-2.c
>>> @@ -0,0 +1,53 @@
>>> +/* { 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.
>
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/powerpc/p10-bifs-vectorize-run-1.c
>>> @@ -0,0 +1,45 @@
>>> +/* { dg-do run } */
>>> +/* { dg-require-effective-target lp64 } */
>> And another.
>>
>>> +#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.
>
>> Okay for trunk with whose lp64 things improved.  Thanks!
>>
> Thanks, v2 has been attached by addressing Bill's and your comments.  :)
>
>
> BR,
> Kewen
> -----
> gcc/ChangeLog:
>
> 	* config/rs6000/rs6000.c (rs6000_builtin_md_vectorized_function): Add
> 	support for built-in functions MISC_BUILTIN_DIVWE, MISC_BUILTIN_DIVWEU,
> 	MISC_BUILTIN_DIVDE, MISC_BUILTIN_DIVDEU, P10_BUILTIN_CFUGED,
> 	P10_BUILTIN_CNTLZDM, P10_BUILTIN_CNTTZDM, P10_BUILTIN_PDEPD and
> 	P10_BUILTIN_PEXTD on Power10.


More information about the Gcc-patches mailing list