zanmato1984 commented on issue #43687:
URL: https://github.com/apache/arrow/issues/43687#issuecomment-2289518423
> > This make sense to me. My concern is should we change like
> > ```diff
> > void AddMinMaxAvx512AggKernels(ScalarAggregateFunction* func) {
> > - AddMinMaxKernels(MinMaxInitAvx512, BaseBinaryTypes(), func,
SimdLevel::AVX2);
> > + AddMinMaxKernels(MinMaxInitAvx512, BaseBinaryTypes(), func,
SimdLevel::AVX512);
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Or this is just intended?
>
> No. Some places in the codebase do this, but it's a mistake: it only
generates unnecessary template instances. The SIMD level parameter should be
the smallest level that the kernel requires.
I'm a little confused about the code too: why specifying AVX2 for
string-like and fixed-size-binary types, as opposed to specifying AVX512 like
for other types.
However I would try to answer myself: for other types, the compiler
generates different SIMD code for AVX512 than for AVX2. So for these kernels,
we have to mark them AVX512-only because an AVX2-only architecture wouldn't
know them. For string-like and fixed-size-binary types, on the other hand, we
are sure that the SIMD code generated by the compiler for both AVX512 and AVX2
are the same (all AVX2-capable?)? So these kernels are actually AVX2-capable,
hence we specify a more relaxing SIMD level (AVX2) for them?
@felipecrv Could you help to confirm if I'm getting this right? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
