[libclc] Move minmag & maxmag to the CLC library #137982
Conversation
| const __CLC_GENTYPE res = __clc_select( | ||
| y, x, | ||
| __CLC_CONVERT_BIT_INTN(__clc_isgreater(__clc_fabs(x), __clc_fabs(y)))); | ||
| return __clc_select( | ||
| res, __clc_fmax(x, y), | ||
| __CLC_CONVERT_BIT_INTN(__clc_isnan(x) || __clc_isnan(y) || | ||
| __clc_isequal(__clc_fabs(x), __clc_fabs(y)))); |
There was a problem hiding this comment.
This seems like a lot more code than is needed. Why the isequal or conversions? This should be 1 fmax, 2 fabs + 2 select?
There was a problem hiding this comment.
Yeah, it does seem excessive. I don't really understand the isequal.
The conversions are necessary because select only accepts conditions of the same size as the vector, but the scalar relational functions unhelpfully return int even for double. There's no implicit conversions on vector types so we need the explicit select. This all comes out in the wash as in LLVM IR it's just i1 types fed from comparison to select anyway.
I've been wondering if we could alleviate this through extra overloads of __clc_select that accept intN conditions across the board, but that might also lead to some confusion.
There was a problem hiding this comment.
This can just use ternary operator, I don't see what the select is adding other than complexity
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
just a move but the implementation could be better
2 participants
No description provided.