On Wed, Nov 27, 2024 at 05:09:17PM +, Devulapalli, Raghuveer wrote:
> LGTM.
Committed.
--
nathan
> Here's what I have staged for commit (except for the commit message, which
> needs some work).
LGTM.
Raghuveer
Here's what I have staged for commit (except for the commit message, which
needs some work).
--
nathan
>From c405f5a41cca9ce5de9c0e69e00d11b16ba93986 Mon Sep 17 00:00:00 2001
From: Nathan Bossart
Date: Tue, 26 Nov 2024 14:17:03 -0600
Subject: [PATCH v6 1/1] Use __attribute__((target(...))) for S
On Mon, Nov 25, 2024 at 11:28:52PM +, Devulapalli, Raghuveer wrote:
>> Thanks. I think my only remaining feedback is that we should probably add a
>> comment to explain why we aren't doing this for ARM yet [0].
>
> Sounds good. Where would you like me to add this comment? Meson.build and
> co
>
> Thanks. I think my only remaining feedback is that we should probably add a
> comment to explain why we aren't doing this for ARM yet [0].
Sounds good. Where would you like me to add this comment? Meson.build and
configure?
On Fri, Nov 22, 2024 at 09:00:01PM +, Devulapalli, Raghuveer wrote:
> Sure. Updated patch.
Thanks. I think my only remaining feedback is that we should probably add
a comment to explain why we aren't doing this for ARM yet [0].
[0] https://postgr.es/m/ZwXsE0KG_wh6_heU%40nathan
--
nathan
> I think we should still use the test program even when __SSE4_2__ is defined,
> but
> we can use that macro to determine whether to use a runtime check. I think
> that
> would keep autoconf and meson consistent.
Sure. Updated patch.
v5-0001-Use-__attribute__-target-sse4.2-for-SSE42-CRC32C.p
On Thu, Nov 21, 2024 at 11:07:59PM +, Devulapalli, Raghuveer wrote:
> Thanks for the review! We can actually leverage meson's built in option
> to check for a macro: cc.get_define('__SSE4_2__') != ''. This should keep
> the logic consistent across configure and meson.
I think we should still
> I think it is in pretty good shape. After a read-through, the only thing
> that stands
> out to me is the difference in configure tests between autoconf and meson. I
> think we should consider picking one approach, although I'm not sure I have a
> strong preference.
Thanks for the review! We
On Wed, Nov 20, 2024 at 05:37:33PM +, Devulapalli, Raghuveer wrote:
> Anymore feedback on this patch? Hoping this is a straightforward one.
I think it is in pretty good shape. After a read-through, the only thing
that stands out to me is the difference in configure tests between autoconf
and
; Shankaran, Akash
>
> Subject: RE: Use __attribute__((target(sse4.2))) for SSE42 CRC32C
>
> V3: With the suggested changes.
>
> Raghuveer
>
> > -Original Message-
> > From: Devulapalli, Raghuveer
> > Sent: Friday, November 8, 2024 10:43 AM
> >
V3: With the suggested changes.
Raghuveer
> -Original Message-
> From: Devulapalli, Raghuveer
> Sent: Friday, November 8, 2024 10:43 AM
> To: Nathan Bossart
> Cc: pgsql-hackers@lists.postgresql.org; Shankaran, Akash
>
> Subject: RE: Use __attribute__((targe
> I believe we expect MSVC builds to use meson at this point, which is probably
> why there's this extra check:
>
> if cc.get_id() == 'msvc'
> cdata.set('USE_SSE42_CRC32C', false)
> cdata.set('USE_SSE42_CRC32C_WITH_RUNTIME_CHECK', 1)
> have_optimized_crc = true
>
Ah, I missed this.
On Fri, Nov 08, 2024 at 05:52:23PM +, Devulapalli, Raghuveer wrote:
>> So, for meson builds, we do test with and without
>> __attribute___((target(..."))),
>> but for autoconf builds, we check for __SSE4_2__ to determine whether we need
>> a runtime check. This difference isn't the fault of y
> -prog = '''
> +sse42_crc_prog = '''
>
> nitpick: Why does this need to be renamed?
Doesn't need to be renamed, but just a better name to describe that the code is
meant for. It will be followed up with avx512_crc_prog in the next patch.
>
> +#ifdef TEST_SSE42_WITH_ATTRIBUTE
> +#if d
-prog = '''
+sse42_crc_prog = '''
nitpick: Why does this need to be renamed?
+#ifdef TEST_SSE42_WITH_ATTRIBUTE
+#if defined(__has_attribute) && __has_attribute (target)
+__attribute__((target("sse4.2")))
+#endif
+#endif
So, for meson builds, we do test with and without
__attribute___((ta
Here is the updated patch with the #if defined(__has_attribute) &&
__has_attribute (target) guard.
> Oh, you are right, sorry.
No worries! 😊
> Is the idea that we will put both "choose" functions in one file and the
> actual
> CRC-32C code in another? I'm okay with that.
Yup, there can only
On Thu, Nov 07, 2024 at 09:30:32PM +, Devulapalli, Raghuveer wrote:
>> # Check for Intel SSE 4.2 intrinsics to do CRC calculations.
>> #
>> -# First check if the _mm_crc32_u8 and _mm_crc32_u64 intrinsics can be used
>> -#
>> with the default compiler flags. If not, check if adding the -msse4
> +__attribute__((target("sse4.2")))
>
> These need to be surrounded with
>
> #if defined(__has_attribute) && __has_attribute (target)
>
> so that we still attempt the check on compilers that don't support it (e.g.,
> MSVC).
I assumed configure was only used on linux and ignored this
+__attribute__((target("sse4.2")))
These need to be surrounded with
#if defined(__has_attribute) && __has_attribute (target)
so that we still attempt the check on compilers that don't support it
(e.g., MSVC).
# Check for Intel SSE 4.2 intrinsics to do CRC calculations.
#
-# First
20 matches
Mail list logo
