Re: [PATCH] Remove redundant builtins for avx512f scalar instructions.
On 11/30/20 9:26 AM, Jakub Jelinek wrote: > On Mon, Nov 30, 2020 at 09:23:15AM -0700, Jeff Law wrote: >> >> On 11/12/20 11:21 PM, Hongyu Wang wrote: >>> Hi >>> >>> Thanks for reminding me about this patch. I didn't remove any existing >>> intrinsics, just remove redundant builtin functions that end-users >>> would not likely to use. >>> >>> Also I'm OK to keep current implementation, in case there might be >>> someone using the builtin directly. >> That seems wise -- we can't reasonably predict if users are using those >> builtins directly. >> >> So if we can clean things up and keep the redundant builtins that seems >> best. Or just leave things as-is. >> >> The other possibility would be to deprecate the redundant builtins this >> release and remove them in gcc-12. I haven't looked at how difficult >> that might be, but the idea here would be to give users a warning if >> they use those builtins directly and enough time to resolve the issue >> before we remove them. > In the past we've removed the builtins without any warning, we state all the > time that the builtins used to implement the intrinsics themselves aren't > supported, only the intrinsics declared in the headers. In that case, I don't mind removing those builtins. jeff
Re: [PATCH] Remove redundant builtins for avx512f scalar instructions.
On Mon, Nov 30, 2020 at 09:23:15AM -0700, Jeff Law wrote: > > > On 11/12/20 11:21 PM, Hongyu Wang wrote: > > Hi > > > > Thanks for reminding me about this patch. I didn't remove any existing > > intrinsics, just remove redundant builtin functions that end-users > > would not likely to use. > > > > Also I'm OK to keep current implementation, in case there might be > > someone using the builtin directly. > That seems wise -- we can't reasonably predict if users are using those > builtins directly. > > So if we can clean things up and keep the redundant builtins that seems > best. Or just leave things as-is. > > The other possibility would be to deprecate the redundant builtins this > release and remove them in gcc-12. I haven't looked at how difficult > that might be, but the idea here would be to give users a warning if > they use those builtins directly and enough time to resolve the issue > before we remove them. In the past we've removed the builtins without any warning, we state all the time that the builtins used to implement the intrinsics themselves aren't supported, only the intrinsics declared in the headers. Jakub
Re: [PATCH] Remove redundant builtins for avx512f scalar instructions.
On 11/12/20 11:21 PM, Hongyu Wang wrote: > Hi > > Thanks for reminding me about this patch. I didn't remove any existing > intrinsics, just remove redundant builtin functions that end-users > would not likely to use. > > Also I'm OK to keep current implementation, in case there might be > someone using the builtin directly. That seems wise -- we can't reasonably predict if users are using those builtins directly. So if we can clean things up and keep the redundant builtins that seems best. Or just leave things as-is. The other possibility would be to deprecate the redundant builtins this release and remove them in gcc-12. I haven't looked at how difficult that might be, but the idea here would be to give users a warning if they use those builtins directly and enough time to resolve the issue before we remove them. jeff
Re: [PATCH] Remove redundant builtins for avx512f scalar instructions.
Hi Thanks for reminding me about this patch. I didn't remove any existing intrinsics, just remove redundant builtin functions that end-users would not likely to use. Also I'm OK to keep current implementation, in case there might be someone using the builtin directly. Jeff Law 于2020年11月13日周五 下午1:43写道: > > > On 12/23/19 10:31 PM, Hongyu Wang wrote: > > Hi: > For avx512f scalar instructions, current builtin function like > __builtin_ia32_*{sd,ss}_round can be replaced by > __builtin_ia32_*{sd,ss}_mask_round with mask parameter set to -1. This > patch did the replacement and remove the corresponding redundant > builtins. > > Bootstrap is ok, make-check ok for i386 target. > Ok for trunk? > > Changelog > > gcc/ > * config/i386/avx512fintrin.h > (_mm_add_round_sd, _mm_add_round_ss): Use > __builtin_ia32_adds?_mask_round builtins instead of > __builtin_ia32_adds?_round. > (_mm_sub_round_sd, _mm_sub_round_ss, > _mm_mul_round_sd, _mm_mul_round_ss, > _mm_div_round_sd, _mm_div_round_ss, > _mm_getexp_sd, _mm_getexp_ss, > _mm_getexp_round_sd, _mm_getexp_round_ss, > _mm_getmant_sd, _mm_getmant_ss, > _mm_getmant_round_sd, _mm_getmant_round_ss, > _mm_max_round_sd, _mm_max_round_ss, > _mm_min_round_sd, _mm_min_round_ss, > _mm_fmadd_round_sd, _mm_fmadd_round_ss, > _mm_fmsub_round_sd, _mm_fmsub_round_ss, > _mm_fnmadd_round_sd, _mm_fnmadd_round_ss, > _mm_fnmsub_round_sd, _mm_fnmsub_round_ss): Likewise. > * config/i386/i386-builtin.def > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > __builtin_ia32_getexpsd128_round, __builtin_ia32_getexpss128_round, > __builtin_ia32_getmantsd_round, __builtin_ia32_getmantss_round, > __builtin_ia32_maxsd_round, __builtin_ia32_maxss_round, > __builtin_ia32_minsd_round, __builtin_ia32_minss_round, > __builtin_ia32_vfmaddsd3_round, > __builtin_ia32_vfmaddss3_round): Remove. > * config/i386/i386-expand.c > (ix86_expand_round_builtin): Remove corresponding case. > > gcc/testsuite/ > * lib/target-supports.exp > (check_effective_target_avx512f): Use > __builtin_ia32_getmantsd_mask_round builtins instead of > __builtin_ia32_getmantsd_round. > *gcc.target/i386/avx-1.c > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > __builtin_ia32_getexpsd128_round, __builtin_ia32_getexpss128_round, > __builtin_ia32_getmantsd_round, __builtin_ia32_getmantss_round, > __builtin_ia32_maxsd_round, __builtin_ia32_maxss_round, > __builtin_ia32_minsd_round, __builtin_ia32_minss_round, > __builtin_ia32_vfmaddsd3_round, > __builtin_ia32_vfmaddss3_round): Remove. > *gcc.target/i386/sse-13.c: Ditto. > *gcc.target/i386/sse-23.c: Ditto. > > So I like the idea of simplifying the implementation of some of the > intrinsics when we can, but ISTM that removing existing intrinsics would be a > mistake since end-users could be using them in their code. I'd think we'd > want to keep the existing APIs, even if we change the implementation under > the hood. > > > Thoughts? > > > jeff > > > Hongyu Wang > > > 0001-Remove-redundant-round-builtins-for-avx512f-scalar-i.patch > > From 9cc4928aad5770c53ff580f5c996092cdaf2f9ba Mon Sep 17 00:00:00 2001 > From: hongyuw1 > Date: Wed, 18 Dec 2019 14:52:54 + > Subject: [PATCH] Remove redundant round builtins for avx512f scalar > instructions > > Changelog > > gcc/ > * config/i386/avx512fintrin.h > (_mm_add_round_sd, _mm_add_round_ss): Use > __builtin_ia32_adds?_mask_round builtins instead of > __builtin_ia32_adds?_round. > (_mm_sub_round_sd, _mm_sub_round_ss, > _mm_mul_round_sd, _mm_mul_round_ss, > _mm_div_round_sd, _mm_div_round_ss, > _mm_getexp_sd, _mm_getexp_ss, > _mm_getexp_round_sd, _mm_getexp_round_ss, > _mm_getmant_sd, _mm_getmant_ss, > _mm_getmant_round_sd, _mm_getmant_round_ss, > _mm_max_round_sd, _mm_max_round_ss, > _mm_min_round_sd, _mm_min_round_ss, > _mm_fmadd_round_sd, _mm_fmadd_round_ss, > _mm_fmsub_round_sd, _mm_fmsub_round_ss, > _mm_fnmadd_round_sd, _mm_fnmadd_round_ss, > _mm_fnmsub_round_sd, _mm_fnmsub_round_ss): Likewise. > * config/i386/i386-builtin.def > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > __builtin_ia32_getexpsd128_round,
Re: [PATCH] Remove redundant builtins for avx512f scalar instructions.
On 12/23/19 10:31 PM, Hongyu Wang wrote: > Hi: > For avx512f scalar instructions, current builtin function like > __builtin_ia32_*{sd,ss}_round can be replaced by > __builtin_ia32_*{sd,ss}_mask_round with mask parameter set to -1. This > patch did the replacement and remove the corresponding redundant > builtins. > > Bootstrap is ok, make-check ok for i386 target. > Ok for trunk? > > Changelog > > gcc/ > * config/i386/avx512fintrin.h > (_mm_add_round_sd, _mm_add_round_ss): Use > __builtin_ia32_adds?_mask_round builtins instead of > __builtin_ia32_adds?_round. > (_mm_sub_round_sd, _mm_sub_round_ss, > _mm_mul_round_sd, _mm_mul_round_ss, > _mm_div_round_sd, _mm_div_round_ss, > _mm_getexp_sd, _mm_getexp_ss, > _mm_getexp_round_sd, _mm_getexp_round_ss, > _mm_getmant_sd, _mm_getmant_ss, > _mm_getmant_round_sd, _mm_getmant_round_ss, > _mm_max_round_sd, _mm_max_round_ss, > _mm_min_round_sd, _mm_min_round_ss, > _mm_fmadd_round_sd, _mm_fmadd_round_ss, > _mm_fmsub_round_sd, _mm_fmsub_round_ss, > _mm_fnmadd_round_sd, _mm_fnmadd_round_ss, > _mm_fnmsub_round_sd, _mm_fnmsub_round_ss): Likewise. > * config/i386/i386-builtin.def > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > __builtin_ia32_getexpsd128_round, __builtin_ia32_getexpss128_round, > __builtin_ia32_getmantsd_round, __builtin_ia32_getmantss_round, > __builtin_ia32_maxsd_round, __builtin_ia32_maxss_round, > __builtin_ia32_minsd_round, __builtin_ia32_minss_round, > __builtin_ia32_vfmaddsd3_round, > __builtin_ia32_vfmaddss3_round): Remove. > * config/i386/i386-expand.c > (ix86_expand_round_builtin): Remove corresponding case. > > gcc/testsuite/ > * lib/target-supports.exp > (check_effective_target_avx512f): Use > __builtin_ia32_getmantsd_mask_round builtins instead of > __builtin_ia32_getmantsd_round. > *gcc.target/i386/avx-1.c > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > __builtin_ia32_getexpsd128_round, __builtin_ia32_getexpss128_round, > __builtin_ia32_getmantsd_round, __builtin_ia32_getmantss_round, > __builtin_ia32_maxsd_round, __builtin_ia32_maxss_round, > __builtin_ia32_minsd_round, __builtin_ia32_minss_round, > __builtin_ia32_vfmaddsd3_round, > __builtin_ia32_vfmaddss3_round): Remove. > *gcc.target/i386/sse-13.c: Ditto. > *gcc.target/i386/sse-23.c: Ditto. So I like the idea of simplifying the implementation of some of the intrinsics when we can, but ISTM that removing existing intrinsics would be a mistake since end-users could be using them in their code. I'd think we'd want to keep the existing APIs, even if we change the implementation under the hood. Thoughts? jeff > Hongyu Wang > > 0001-Remove-redundant-round-builtins-for-avx512f-scalar-i.patch > > From 9cc4928aad5770c53ff580f5c996092cdaf2f9ba Mon Sep 17 00:00:00 2001 > From: hongyuw1 > Date: Wed, 18 Dec 2019 14:52:54 + > Subject: [PATCH] Remove redundant round builtins for avx512f scalar > instructions > > Changelog > > gcc/ > * config/i386/avx512fintrin.h > (_mm_add_round_sd, _mm_add_round_ss): Use >__builtin_ia32_adds?_mask_round builtins instead of > __builtin_ia32_adds?_round. > (_mm_sub_round_sd, _mm_sub_round_ss, > _mm_mul_round_sd, _mm_mul_round_ss, > _mm_div_round_sd, _mm_div_round_ss, > _mm_getexp_sd, _mm_getexp_ss, > _mm_getexp_round_sd, _mm_getexp_round_ss, > _mm_getmant_sd, _mm_getmant_ss, > _mm_getmant_round_sd, _mm_getmant_round_ss, > _mm_max_round_sd, _mm_max_round_ss, > _mm_min_round_sd, _mm_min_round_ss, > _mm_fmadd_round_sd, _mm_fmadd_round_ss, > _mm_fmsub_round_sd, _mm_fmsub_round_ss, > _mm_fnmadd_round_sd, _mm_fnmadd_round_ss, > _mm_fnmsub_round_sd, _mm_fnmsub_round_ss): Likewise. > * config/i386/i386-builtin.def > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > __builtin_ia32_getexpsd128_round, __builtin_ia32_getexpss128_round, > __builtin_ia32_getmantsd_round, __builtin_ia32_getmantss_round, > __builtin_ia32_maxsd_round, __builtin_ia32_maxss_round, > __builtin_ia32_minsd_round,
Re: [PATCH] Remove redundant builtins for avx512f scalar instructions.
For sure. Jeff Law 于2020年1月15日周三 上午4:48写道: > > On Tue, 2019-12-24 at 13:31 +0800, Hongyu Wang wrote: > > Hi: > > For avx512f scalar instructions, current builtin function like > > __builtin_ia32_*{sd,ss}_round can be replaced by > > __builtin_ia32_*{sd,ss}_mask_round with mask parameter set to -1. This > > patch did the replacement and remove the corresponding redundant > > builtins. > > > > Bootstrap is ok, make-check ok for i386 target. > > Ok for trunk? > > > > Changelog > > > > gcc/ > > * config/i386/avx512fintrin.h > > (_mm_add_round_sd, _mm_add_round_ss): Use > > __builtin_ia32_adds?_mask_round builtins instead of > > __builtin_ia32_adds?_round. > > (_mm_sub_round_sd, _mm_sub_round_ss, > > _mm_mul_round_sd, _mm_mul_round_ss, > > _mm_div_round_sd, _mm_div_round_ss, > > _mm_getexp_sd, _mm_getexp_ss, > > _mm_getexp_round_sd, _mm_getexp_round_ss, > > _mm_getmant_sd, _mm_getmant_ss, > > _mm_getmant_round_sd, _mm_getmant_round_ss, > > _mm_max_round_sd, _mm_max_round_ss, > > _mm_min_round_sd, _mm_min_round_ss, > > _mm_fmadd_round_sd, _mm_fmadd_round_ss, > > _mm_fmsub_round_sd, _mm_fmsub_round_ss, > > _mm_fnmadd_round_sd, _mm_fnmadd_round_ss, > > _mm_fnmsub_round_sd, _mm_fnmsub_round_ss): Likewise. > > * config/i386/i386-builtin.def > > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > > __builtin_ia32_getexpsd128_round, __builtin_ia32_getexpss128_round, > > __builtin_ia32_getmantsd_round, __builtin_ia32_getmantss_round, > > __builtin_ia32_maxsd_round, __builtin_ia32_maxss_round, > > __builtin_ia32_minsd_round, __builtin_ia32_minss_round, > > __builtin_ia32_vfmaddsd3_round, > > __builtin_ia32_vfmaddss3_round): Remove. > > * config/i386/i386-expand.c > > (ix86_expand_round_builtin): Remove corresponding case. > This doesn't really look like a bugfix to me. Can it wait for gcc-11? > > jeff > > >
Re: [PATCH] Remove redundant builtins for avx512f scalar instructions.
On Tue, 2019-12-24 at 13:31 +0800, Hongyu Wang wrote: > Hi: > For avx512f scalar instructions, current builtin function like > __builtin_ia32_*{sd,ss}_round can be replaced by > __builtin_ia32_*{sd,ss}_mask_round with mask parameter set to -1. This > patch did the replacement and remove the corresponding redundant > builtins. > > Bootstrap is ok, make-check ok for i386 target. > Ok for trunk? > > Changelog > > gcc/ > * config/i386/avx512fintrin.h > (_mm_add_round_sd, _mm_add_round_ss): Use > __builtin_ia32_adds?_mask_round builtins instead of > __builtin_ia32_adds?_round. > (_mm_sub_round_sd, _mm_sub_round_ss, > _mm_mul_round_sd, _mm_mul_round_ss, > _mm_div_round_sd, _mm_div_round_ss, > _mm_getexp_sd, _mm_getexp_ss, > _mm_getexp_round_sd, _mm_getexp_round_ss, > _mm_getmant_sd, _mm_getmant_ss, > _mm_getmant_round_sd, _mm_getmant_round_ss, > _mm_max_round_sd, _mm_max_round_ss, > _mm_min_round_sd, _mm_min_round_ss, > _mm_fmadd_round_sd, _mm_fmadd_round_ss, > _mm_fmsub_round_sd, _mm_fmsub_round_ss, > _mm_fnmadd_round_sd, _mm_fnmadd_round_ss, > _mm_fnmsub_round_sd, _mm_fnmsub_round_ss): Likewise. > * config/i386/i386-builtin.def > (__builtin_ia32_addsd_round, __builtin_ia32_addss_round, > __builtin_ia32_subsd_round, __builtin_ia32_subss_round, > __builtin_ia32_mulsd_round, __builtin_ia32_mulss_round, > __builtin_ia32_divsd_round, __builtin_ia32_divss_round, > __builtin_ia32_getexpsd128_round, __builtin_ia32_getexpss128_round, > __builtin_ia32_getmantsd_round, __builtin_ia32_getmantss_round, > __builtin_ia32_maxsd_round, __builtin_ia32_maxss_round, > __builtin_ia32_minsd_round, __builtin_ia32_minss_round, > __builtin_ia32_vfmaddsd3_round, > __builtin_ia32_vfmaddss3_round): Remove. > * config/i386/i386-expand.c > (ix86_expand_round_builtin): Remove corresponding case. This doesn't really look like a bugfix to me. Can it wait for gcc-11? jeff >