On Tue, Jan 02, 2024 at 12:50:04PM -0500, Tom Lane wrote: > The patch needs better comments (as in, more than "none whatsoever").
Yes, will do. > Also, do you really want to structure the header so that USE_SSE2 > doesn't get defined? In that case you are committing to provide > an AVX2 replacement every single place that there's USE_SSE2, which > doesn't seem like a great thing to require. OTOH, maybe there's > no choice given than we need a different definition for Vector8 and > Vector32? Yeah, the precedent is to use these abstracted types elsewhere so that any SIMD-related improvements aren't limited to one architecture. There are a couple of places that do explicitly check for USE_NO_SIMD, though. Maybe there's an eventual use-case for using SSE2 intrinsics even when you have AVX2 support, but for now, ensuring we have an AVX2 replacement for everything doesn't seem particularly burdensome. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
