Hi Richi, Thanks for the review!
在 2026/6/18 19:31, Richard Biener 写道: > On Thu, Jun 18, 2026 at 1:18 PM Kewen Lin <[email protected]> wrote: >> >> Hi, >> >> This patch is authored-by: Zhongjie Guo <[email protected]> >> >> AVX512 vector comparisons produce mask results, but the instructions >> operate on vector operands. The instruction cost should therefore be >> based on the operand vector mode rather than the mask result mode. >> >> The mismatch is especially visible on AVX512 split-regs targets, where >> ix86_vec_cost scales 512-bit vector operations. Using the mask result >> type misses that scaling and can make 512-bit vectorization look too >> cheap. >> >> For mask-producing comparisons, use the operand vector type recorded on >> the SLP children. If no operand type is recorded, derive a related vector >> type from the scalar comparison type without changing the vectorizer's >> chosen modes. Mask-vs-mask comparisons keep their mask result type and >> continue to be costed as mask operations. >> >> Bootstrapped and regtested on a hygon c86-4g-m7 machine. >> >> Is it ok for trunk? >> >> BR, >> Kewen >> ----- >> >> gcc/ChangeLog: >> >> * config/i386/i386.cc (ix86_vector_costs::add_stmt_cost): Use the >> comparison operand vector mode for mask-producing vector comparisons. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/i386/vect-compare-cost.c: New test. >> >> Signed-off-by: Zhongjie Guo <[email protected]> >> --- >> gcc/config/i386/i386.cc | 21 +++++++++++++++++++ >> .../gcc.target/i386/vect-compare-cost.c | 14 +++++++++++++ >> 2 files changed, 35 insertions(+) >> create mode 100644 gcc/testsuite/gcc.target/i386/vect-compare-cost.c >> >> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc >> index 2945081234b..53d47596fb7 100644 >> --- a/gcc/config/i386/i386.cc >> +++ b/gcc/config/i386/i386.cc >> @@ -26497,6 +26497,27 @@ ix86_vector_costs::add_stmt_cost (int count, >> vect_cost_for_stmt kind, >> default: >> if (truth_value_p (subcode)) >> { >> + if (kind == vector_stmt >> + && vectype >> + && VECTOR_BOOLEAN_TYPE_P (vectype)) >> + { >> + tree operand_vectype >> + = SLP_TREE_VECTYPE (SLP_TREE_CHILDREN (node)[0]); >> + if (!operand_vectype) > > when does this happen? (it should not) I'll note that truth_value_p above is I think it follows the handlings in vectorizable_comparison_1, which checks for (!vectype) after vect_is_simple_use applied on rhs1/rhs2. > worrying as it would include TRUTH_NOT_EXPR (unary). Of course only > tcc_comparison ever happens here so can you replace truth_value_p > with TREE_CODE_CLASS (subcode) == tcc_comparison? Good point! Will do. > >> + operand_vectype >> + = SLP_TREE_VECTYPE (SLP_TREE_CHILDREN (node)[1]); >> + >> + if (!operand_vectype) > > See above. > >> + if (tree cmp_type = vect_comparison_type (stmt_info)) >> + if (!VECT_SCALAR_BOOLEAN_TYPE_P (cmp_type)) >> + operand_vectype >> + = get_related_vectype_for_scalar_type >> + (m_vinfo->vector_mode, cmp_type); >> + >> + if (operand_vectype) >> + mode = TYPE_MODE (operand_vectype); > > IMO setting mode should either happen earlier and unconditional > (also for scalar compares) or you should set cost directly. > >> + } >> + >> if (SSE_FLOAT_MODE_SSEMATH_OR_HFBF_P (mode)) > > But I agree in general that 'mode' for compares should be the mode of > the operands. > The > > else if (X87_FLOAT_MODE_P (mode)) > /* fcmp + setcc. */ > stmt_cost = ix86_cost->fadd + ix86_cost->add; > > path is similarly affected for scalar code, so I'd prefer a more global 'mode' > adjustment. Do you suggest something like: diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index b1fd86c2b32..964c4bdef90 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -26510,8 +26510,15 @@ ix86_vector_costs::add_stmt_cost (int count, vect_cost_for_stmt kind, break; default: - if (truth_value_p (subcode)) + if (TREE_CODE_CLASS (subcode) == tcc_comparison) { + tree op_type = vect_comparison_type (stmt_info); + if (VECTOR_TYPE_P (vectype)) + op_type + = get_related_vectype_for_scalar_type (m_vinfo->vector_mode, + op_type); + mode = TYPE_MODE (op_type); + if (SSE_FLOAT_MODE_SSEMATH_OR_HFBF_P (mode)) /* CMPccS? insructions are cheap, so use sse_op. While they produce a mask which may need to be turned to 0/1 by and, ? // it's untested yet. BR, Kewen
