github-actions[bot] commented on code in PR #66276:
URL: https://github.com/apache/doris/pull/66276#discussion_r3681080488
##########
be/src/exprs/function/least_greast.cpp:
##########
@@ -134,30 +134,29 @@ struct CompareMultiImpl {
if constexpr (std::is_same_v<ColumnType, ColumnDecimal128V2>) {
for (size_t i = 0; i < input_rows_count; ++i) {
- result_raw_data[i] =
-
Op<TYPE_DECIMALV2>::apply(column_raw_data[index_check_const(i, ArgConst)],
- result_raw_data[i])
- ? column_raw_data[index_check_const(i,
ArgConst)]
- : result_raw_data[i];
+ if
(Op<TYPE_DECIMALV2>::apply(column_raw_data[index_check_const(i, ArgConst)],
+ result_raw_data[i])) {
+ result_raw_data[i] = column_raw_data[index_check_const(i,
ArgConst)];
+ }
}
} else if constexpr (std::is_same_v<ColumnType, ColumnDecimal32> ||
std::is_same_v<ColumnType, ColumnDecimal64> ||
std::is_same_v<ColumnType, ColumnDecimal128V3> ||
std::is_same_v<ColumnType, ColumnDecimal256>) {
for (size_t i = 0; i < input_rows_count; ++i) {
- result_raw_data[i] =
- Op<PType>::apply(column_raw_data[index_check_const(i,
ArgConst)].value,
- result_raw_data[i].value)
- ? column_raw_data[index_check_const(i,
ArgConst)]
- : result_raw_data[i];
+ if (Op<PType>::apply(column_raw_data[index_check_const(i,
ArgConst)].value,
+ result_raw_data[i].value)) {
+ result_raw_data[i] = column_raw_data[index_check_const(i,
ArgConst)];
+ }
}
} else {
+ // A ternary self-assignment triggers a false UnsafeDep in Clang's
loop vectorizer.
+ // See https://github.com/llvm/llvm-project/issues/212787.
for (size_t i = 0; i < input_rows_count; ++i) {
- result_raw_data[i] =
- Op<PType>::apply(column_raw_data[index_check_const(i,
ArgConst)],
- result_raw_data[i])
- ? column_raw_data[index_check_const(i,
ArgConst)]
- : result_raw_data[i];
+ if (Op<PType>::apply(column_raw_data[index_check_const(i,
ArgConst)],
Review Comment:
[P1] Preserve vector min/max codegen for byte/word least/greatest
On Doris's default Release x86 target (`-O3 -mavx2`), this branch is
instantiated for BOOLEAN/TINYINT/SMALLINT. With Clang 18, the old ternary
generates compact, four-way-unrolled `vpminub`/`vpminsb`/`vpminsw` loops (and
symmetric max instructions), but the new conditional store drops to interleave
1 and expands each vector comparison into `vpextrb`/`vpextrw` tests, branches,
and individual byte/word stores because AVX2 has no masked store for those
widths. Thus this performance-only change makes these common least/greatest
paths substantially worse. Please keep the selection/ternary form for widths
that already vectorize, or otherwise restrict the rewrite by measured
type/target, and add a representative codegen or benchmark check.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
