From 4e2830c9efc07b7d0bd2cf2143ab4cf44a3251b5 Mon Sep 17 00:00:00 2001 From: Faith Ekstrand Date: Tue, 15 Aug 2023 10:59:11 -0500 Subject: [PATCH] nir: Clean up nir_op_is_vec() and its callers The nir_op_is_vec() helper I added in 842338e2f0bd ("nir: Add a nir_op_is_vec helper") treats nir_op_mov as a vec even though the semanitcs of the two are different. In retrospect, this was a mistake as the previous three fixup commits show. This commit splits the helper into two: nir_op_is_vec() and nir_op_is_vec_or_mov() and uses the appropriate helper at each call site. Hopefully, this rename will encurage any future users of these helpers to think about nir_op_mov as separate from nir_op_vecN and we can avoid these bugs. Reviewed-by: Alyssa Rosenzweig Part-of: --- src/amd/common/ac_nir_lower_ngg.c | 2 +- src/asahi/compiler/agx_compile.c | 11 +++++---- src/asahi/compiler/agx_nir_opt_preamble.c | 2 +- src/compiler/nir/nir.c | 17 ++++--------- src/compiler/nir/nir.h | 27 ++++++++++++++++++--- src/compiler/nir/nir_lower_phis_to_scalar.c | 2 +- src/compiler/nir/nir_lower_vec_to_regs.c | 2 +- src/compiler/nir/nir_opt_copy_propagate.c | 2 +- src/compiler/nir/nir_opt_if.c | 2 +- src/compiler/nir/nir_opt_shrink_vectors.c | 2 +- src/compiler/nir/nir_opt_sink.c | 2 +- src/compiler/nir/nir_opt_undef.c | 4 +-- src/compiler/nir/tests/vars_tests.cpp | 2 +- src/gallium/auxiliary/nir/nir_to_tgsi.c | 3 --- src/microsoft/compiler/nir_to_dxil.c | 2 +- 15 files changed, 47 insertions(+), 35 deletions(-) diff --git a/src/amd/common/ac_nir_lower_ngg.c b/src/amd/common/ac_nir_lower_ngg.c index d98833e50d6..1ba5517f004 100644 --- a/src/amd/common/ac_nir_lower_ngg.c +++ b/src/amd/common/ac_nir_lower_ngg.c @@ -793,7 +793,7 @@ remove_extra_pos_output(nir_builder *b, nir_instr *instr, void *state) if (store_val->parent_instr->type == nir_instr_type_alu) { nir_alu_instr *alu = nir_instr_as_alu(store_val->parent_instr); - if (nir_op_is_vec(alu->op)) { + if (nir_op_is_vec_or_mov(alu->op)) { /* Output store uses a vector, we can easily rewrite uses of each vector element. */ unsigned num_vec_src = 0; diff --git a/src/asahi/compiler/agx_compile.c b/src/asahi/compiler/agx_compile.c index 405f75bdba9..9baddd973bc 100644 --- a/src/asahi/compiler/agx_compile.c +++ b/src/asahi/compiler/agx_compile.c @@ -1229,11 +1229,12 @@ agx_emit_alu(agx_builder *b, nir_alu_instr *instr) unsigned src_sz = srcs ? nir_src_bit_size(instr->src[0].src) : 0; ASSERTED unsigned comps = instr->def.num_components; - assert(comps == 1 || nir_op_is_vec(instr->op)); - assert(sz == 1 || - ((nir_op_is_vec(instr->op) || is_conversion_to_8bit(instr->op)) && - sz == 8) || - sz == 16 || sz == 32 || sz == 64); + assert(comps == 1 || nir_op_is_vec_or_mov(instr->op)); + assert( + sz == 1 || + ((nir_op_is_vec_or_mov(instr->op) || is_conversion_to_8bit(instr->op)) && + sz == 8) || + sz == 16 || sz == 32 || sz == 64); agx_index dst = agx_def_index(&instr->def); agx_index s0 = srcs > 0 ? agx_alu_src_index(b, instr->src[0]) : agx_null(); diff --git a/src/asahi/compiler/agx_nir_opt_preamble.c b/src/asahi/compiler/agx_nir_opt_preamble.c index cf765edda0d..65515753788 100644 --- a/src/asahi/compiler/agx_nir_opt_preamble.c +++ b/src/asahi/compiler/agx_nir_opt_preamble.c @@ -39,7 +39,7 @@ instr_cost(nir_instr *instr, const void *data) case nir_instr_type_alu: /* We optimistically assume that moves get coalesced */ - if (nir_op_is_vec(nir_instr_as_alu(instr)->op)) + if (nir_op_is_vec_or_mov(nir_instr_as_alu(instr)->op)) return 0.0; else return 2.0; diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c index 289ecd0c3f5..b2d6bbd7a87 100644 --- a/src/compiler/nir/nir.c +++ b/src/compiler/nir/nir.c @@ -2597,9 +2597,9 @@ nir_chase_binding(nir_src rsrc) } /* Skip copies and trimming. Trimming can appear as nir_op_mov instructions - * when removing the offset from addresses. We also consider nir_op_is_vec() - * instructions to skip trimming of vec2_index_32bit_offset addresses after - * lowering ALU to scalar. + * when removing the offset from addresses. We also consider + * nir_op_is_vec_or_mov() instructions to skip trimming of + * vec2_index_32bit_offset addresses after lowering ALU to scalar. */ unsigned num_components = nir_src_num_components(rsrc); while (true) { @@ -2708,12 +2708,6 @@ nir_get_binding_variable(nir_shader *shader, nir_binding binding) return binding_var; } -bool -nir_alu_instr_is_copy(nir_alu_instr *instr) -{ - return nir_op_is_vec(instr->op); -} - nir_scalar nir_scalar_chase_movs(nir_scalar s) { @@ -2792,10 +2786,10 @@ nir_get_glsl_base_type_for_nir_type(nir_alu_type base_type) } nir_op -nir_op_vec(unsigned components) +nir_op_vec(unsigned num_components) { /* clang-format off */ - switch (components) { + switch (num_components) { case 1: return nir_op_mov; case 2: return nir_op_vec2; case 3: return nir_op_vec3; @@ -2812,7 +2806,6 @@ bool nir_op_is_vec(nir_op op) { switch (op) { - case nir_op_mov: case nir_op_vec2: case nir_op_vec3: case nir_op_vec4: diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h index 264e4cca4c8..ae036e886c4 100644 --- a/src/compiler/nir/nir.h +++ b/src/compiler/nir/nir.h @@ -1231,12 +1231,35 @@ nir_atomic_op_type(nir_atomic_op op) unreachable("Invalid nir_atomic_op"); } +/** Returns nir_op_vec or nir_op_mov if num_components == 1 + * + * This is subtly different from nir_op_is_vec() which returns false for + * nir_op_mov. Returning nir_op_mov from nir_op_vec() when num_components == 1 + * makes sense under the assumption that the num_components of the resulting + * nir_def will same as what is passed in here because a single-component mov + * is effectively a vec1. However, if alu->def.num_components > 1, nir_op_mov + * has different semantics from nir_op_vec* so so code which detects "is this + * a vec?" typically needs to handle nir_op_mov separate from nir_op_vecN. + * + * In the unlikely case where you can handle nir_op_vecN and nir_op_mov + * together, use nir_op_is_vec_or_mov(). + */ nir_op -nir_op_vec(unsigned components); +nir_op_vec(unsigned num_components); +/** Returns true if this op is one of nir_op_vec* + * + * Returns false for nir_op_mov. See nir_op_vec() for more details. + */ bool nir_op_is_vec(nir_op op); +static inline bool +nir_op_is_vec_or_mov(nir_op op) +{ + return op == nir_op_mov || nir_op_is_vec(op); +} + static inline bool nir_is_float_control_signed_zero_inf_nan_preserve(unsigned execution_mode, unsigned bit_size) { @@ -1449,8 +1472,6 @@ typedef struct nir_alu_instr { void nir_alu_src_copy(nir_alu_src *dest, const nir_alu_src *src, nir_alu_instr *instr); -bool nir_alu_instr_is_copy(nir_alu_instr *instr); - /* is this source channel used? */ bool nir_alu_instr_channel_used(const nir_alu_instr *instr, unsigned src, diff --git a/src/compiler/nir/nir_lower_phis_to_scalar.c b/src/compiler/nir/nir_lower_phis_to_scalar.c index a88a5ee24c7..a6c5f9b3a94 100644 --- a/src/compiler/nir/nir_lower_phis_to_scalar.c +++ b/src/compiler/nir/nir_lower_phis_to_scalar.c @@ -61,7 +61,7 @@ is_phi_src_scalarizable(nir_phi_src *src, * are ok too. */ return nir_op_infos[src_alu->op].output_size == 0 || - nir_op_is_vec(src_alu->op); + nir_op_is_vec_or_mov(src_alu->op); } case nir_instr_type_phi: diff --git a/src/compiler/nir/nir_lower_vec_to_regs.c b/src/compiler/nir/nir_lower_vec_to_regs.c index b636c156aa8..a79295a5840 100644 --- a/src/compiler/nir/nir_lower_vec_to_regs.c +++ b/src/compiler/nir/nir_lower_vec_to_regs.c @@ -195,7 +195,7 @@ lower(nir_builder *b, nir_instr *instr, void *data_) return false; nir_alu_instr *vec = nir_instr_as_alu(instr); - if (vec->op == nir_op_mov || !nir_op_is_vec(vec->op)) + if (!nir_op_is_vec(vec->op)) return false; unsigned num_components = vec->def.num_components; diff --git a/src/compiler/nir/nir_opt_copy_propagate.c b/src/compiler/nir/nir_opt_copy_propagate.c index 6945e20d768..da9e5bc4f46 100644 --- a/src/compiler/nir/nir_opt_copy_propagate.c +++ b/src/compiler/nir/nir_opt_copy_propagate.c @@ -128,7 +128,7 @@ copy_prop_instr(nir_instr *instr) nir_alu_instr *mov = nir_instr_as_alu(instr); - if (!nir_alu_instr_is_copy(mov)) + if (!nir_op_is_vec_or_mov(mov->op)) return false; bool progress = false; diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c index df7850d2fb8..2322490f623 100644 --- a/src/compiler/nir/nir_opt_if.c +++ b/src/compiler/nir/nir_opt_if.c @@ -399,7 +399,7 @@ opt_split_alu_of_phi(nir_builder *b, nir_loop *loop) * to loop unrolling not recognizing loop termintators, and type * conversions also lead to regressions. */ - if (nir_op_is_vec(alu->op) || + if (nir_op_is_vec_or_mov(alu->op) || nir_alu_instr_is_comparison(alu) || alu_instr_is_type_conversion(alu)) continue; diff --git a/src/compiler/nir/nir_opt_shrink_vectors.c b/src/compiler/nir/nir_opt_shrink_vectors.c index c204e9b2400..adb1282511e 100644 --- a/src/compiler/nir/nir_opt_shrink_vectors.c +++ b/src/compiler/nir/nir_opt_shrink_vectors.c @@ -439,7 +439,7 @@ opt_shrink_vectors_phi(nir_builder *b, nir_phi_instr *instr) /* However, even if the instruction only points back at the phi, we still * need to check that the swizzles are trivial. */ - if (nir_op_is_vec(alu->op) && alu->op != nir_op_mov) { + if (nir_op_is_vec(alu->op)) { if (src_idx != alu->src[src_idx].swizzle[0]) { mask |= src_read_mask; } diff --git a/src/compiler/nir/nir_opt_sink.c b/src/compiler/nir/nir_opt_sink.c index 5b6164eaad1..f6b4cb194ec 100644 --- a/src/compiler/nir/nir_opt_sink.c +++ b/src/compiler/nir/nir_opt_sink.c @@ -44,7 +44,7 @@ nir_can_move_instr(nir_instr *instr, nir_move_options options) return options & nir_move_const_undef; } case nir_instr_type_alu: { - if (nir_op_is_vec(nir_instr_as_alu(instr)->op) || + if (nir_op_is_vec_or_mov(nir_instr_as_alu(instr)->op) || nir_instr_as_alu(instr)->op == nir_op_b2i32) return options & nir_move_copies; if (nir_alu_instr_is_comparison(nir_instr_as_alu(instr))) diff --git a/src/compiler/nir/nir_opt_undef.c b/src/compiler/nir/nir_opt_undef.c index ce950f4c039..8a5c71fab3e 100644 --- a/src/compiler/nir/nir_opt_undef.c +++ b/src/compiler/nir/nir_opt_undef.c @@ -72,7 +72,7 @@ opt_undef_csel(nir_alu_instr *instr) static bool opt_undef_vecN(nir_builder *b, nir_alu_instr *alu) { - if (!nir_op_is_vec(alu->op)) + if (!nir_op_is_vec_or_mov(alu->op)) return false; for (unsigned i = 0; i < nir_op_infos[alu->op].num_inputs; i++) { @@ -103,7 +103,7 @@ nir_get_undef_mask(nir_def *def) unsigned undef = 0; /* nir_op_mov of undef is handled by opt_undef_vecN() */ - if (nir_op_is_vec(alu->op) && alu->op != nir_op_mov) { + if (nir_op_is_vec(alu->op)) { for (int i = 0; i < nir_op_infos[alu->op].num_inputs; i++) { if (alu->src[i].src.ssa->parent_instr->type == nir_instr_type_undef) { diff --git a/src/compiler/nir/tests/vars_tests.cpp b/src/compiler/nir/tests/vars_tests.cpp index 341b87afe42..53bf6cf97ce 100644 --- a/src/compiler/nir/tests/vars_tests.cpp +++ b/src/compiler/nir/tests/vars_tests.cpp @@ -1948,7 +1948,7 @@ vec_src_comp_as_int(nir_src src, unsigned comp) return nir_src_comp_as_int(src, comp); nir_scalar s = { src.ssa, comp }; - assert(nir_op_is_vec(nir_scalar_alu_op(s))); + assert(nir_op_is_vec_or_mov(nir_scalar_alu_op(s))); return nir_scalar_as_int(nir_scalar_chase_alu_src(s, comp)); } diff --git a/src/gallium/auxiliary/nir/nir_to_tgsi.c b/src/gallium/auxiliary/nir/nir_to_tgsi.c index 5c963eb11ac..cde952f8bd2 100644 --- a/src/gallium/auxiliary/nir/nir_to_tgsi.c +++ b/src/gallium/auxiliary/nir/nir_to_tgsi.c @@ -570,9 +570,6 @@ ntt_extract_const_src_offset(nir_src *src) /* We'd like to reuse nir_scalar_chase_movs(), but it assumes SSA and that * seems reasonable for something used in inner loops of the compiler. */ - if (!nir_alu_instr_is_copy(alu)) - return 0; - if (alu->op == nir_op_mov) { s.def = alu->src[0].src.ssa; s.comp = alu->src[0].swizzle[s.comp]; diff --git a/src/microsoft/compiler/nir_to_dxil.c b/src/microsoft/compiler/nir_to_dxil.c index 0c0ccb37232..92926717d95 100644 --- a/src/microsoft/compiler/nir_to_dxil.c +++ b/src/microsoft/compiler/nir_to_dxil.c @@ -6119,7 +6119,7 @@ lower_bit_size_callback(const nir_instr* instr, void *data) if (nir_op_infos[alu->op].is_conversion) return 0; - if (nir_alu_instr_is_copy(alu)) + if (nir_op_is_vec_or_mov(alu->op)) return 0; unsigned num_inputs = nir_op_infos[alu->op].num_inputs;