From ffdc310bf17480f4d1b4381e7fccac4bca3602e9 Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Wed, 25 Mar 2026 09:50:25 -0700 Subject: [PATCH] brw/const: Don't allow type changes when accumulators are involved Integer accumulators and float accumulators do not occupy the same bits, so the types cannot be arbitrarily changed. No shader-db or fossil-db changes on any Intel platform. v2: Use is_accumulator() instead if brw_reg_is_arf(). Add an extra test to show the desired behavior when an accumulator is not involved. Suggested by Caio. Fixes: 64c251bb3af ("intel/fs: Combine constants for SEL instructions too") Reviewed-by: Caio Oliveira Part-of: --- .../brw/brw_opt_combine_constants.cpp | 10 +- .../brw/test_opt_combine_constants.cpp | 144 ++++++++++++++++++ 2 files changed, 151 insertions(+), 3 deletions(-) diff --git a/src/intel/compiler/brw/brw_opt_combine_constants.cpp b/src/intel/compiler/brw/brw_opt_combine_constants.cpp index 3b2f15852e2..e0fdbf3d2a2 100644 --- a/src/intel/compiler/brw/brw_opt_combine_constants.cpp +++ b/src/intel/compiler/brw/brw_opt_combine_constants.cpp @@ -1149,14 +1149,18 @@ add_candidate_immediate(struct table *table, brw_inst *inst, unsigned ip, } /* It is safe to change the type of the operands of a select instruction - * that has no conditional modifier, no source modifiers, and no saturate - * modifer. + * that has no conditional modifier, no source modifiers, no saturate + * modifier, and no accumulators are used. The types of accumulator accesses + * cannot be arbitrarily changed. */ if (inst->opcode == BRW_OPCODE_SEL && inst->conditional_mod == BRW_CONDITIONAL_NONE && !inst->src[0].negate && !inst->src[0].abs && !inst->src[1].negate && !inst->src[1].abs && - !inst->saturate) { + !inst->saturate && + !inst->src[0].is_accumulator() && + !inst->src[1].is_accumulator() && + !inst->dst.is_accumulator()) { v->type = either_type; } } diff --git a/src/intel/compiler/brw/test_opt_combine_constants.cpp b/src/intel/compiler/brw/test_opt_combine_constants.cpp index 75f0a323237..d615e4f8c35 100644 --- a/src/intel/compiler/brw/test_opt_combine_constants.cpp +++ b/src/intel/compiler/brw/test_opt_combine_constants.cpp @@ -61,3 +61,147 @@ TEST_F(CombineConstantsTest, DoContainingDo) EXPECT_SHADERS_MATCH(bld, exp); } + +TEST_F(CombineConstantsTest, sel_f_integer_negation) +{ + brw_builder bld = make_shader(MESA_SHADER_COMPUTE); + brw_builder exp = make_shader(MESA_SHADER_COMPUTE); + + brw_reg dst0 = vgrf(bld, exp, BRW_TYPE_F); + brw_reg dst1 = vgrf(bld, exp, BRW_TYPE_F); + brw_reg src0 = vgrf(bld, exp, BRW_TYPE_F); + brw_reg src1 = vgrf(bld, exp, BRW_TYPE_F); + brw_reg tmp1 = vgrf(bld, exp, BRW_TYPE_D); + + /* src0/src1 are not relevant to the SEL instructions, so they are filled + * with garbage. + */ + bld.BFREV(retype(src0, BRW_TYPE_UD), brw_imm_ud(1)); + bld.BFREV(retype(src1, BRW_TYPE_UD), brw_imm_ud(2)); + + bld.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src0, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + /* Both SEL instructions have F types. src0 of the first SEL and src1 of + * the second SEL are the integer negations of each other. Constant + * combining is expected to change the type on one of the SEL to D, store a + * single integer value in a register, and use integer negation in a SEL + * source to generate the other value. + */ + bld.SEL(dst0, + /* The type cast here enables using values that are integer negations + * of each other. + */ + retype(brw_imm_d(1), BRW_TYPE_F), + brw_imm_f(2.0)) + ->predicate = BRW_PREDICATE_NORMAL; + + bld.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src1, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + bld.SEL(dst1, + /* The type cast here enables using values that are integer negations + * of each other. + */ + retype(brw_imm_d(-1), BRW_TYPE_F), + brw_imm_f(3.0)) + ->predicate = BRW_PREDICATE_NORMAL; + + EXPECT_PROGRESS(brw_opt_combine_constants, bld); + + exp.BFREV(retype(src0, BRW_TYPE_UD), brw_imm_ud(1)); + exp.BFREV(retype(src1, BRW_TYPE_UD), brw_imm_ud(2)); + + exp.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src0, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + tmp1.stride = 0; + exp.uniform().MOV(tmp1, brw_imm_d(1)); + exp.SEL(dst0, + retype(tmp1, BRW_TYPE_F), + brw_imm_f(2.0)) + ->predicate = BRW_PREDICATE_NORMAL; + + exp.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src1, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + exp.SEL(retype(dst1, BRW_TYPE_D), + /* Note the negation here to convert 1 to -1. */ + negate(tmp1), + retype(brw_imm_f(3.0), BRW_TYPE_D)) + ->predicate = BRW_PREDICATE_NORMAL; + + EXPECT_SHADERS_MATCH(bld, exp); +} + +TEST_F(CombineConstantsTest, sel_to_accumulator) +{ + brw_builder bld = make_shader(MESA_SHADER_COMPUTE); + brw_builder exp = make_shader(MESA_SHADER_COMPUTE); + + brw_reg src0 = vgrf(bld, exp, BRW_TYPE_F); + brw_reg src1 = vgrf(bld, exp, BRW_TYPE_F); + brw_reg tmp1 = vgrf(bld, exp, BRW_TYPE_D); + brw_reg acc0 = retype(brw_acc_reg(8 * reg_unit(devinfo)), + BRW_TYPE_F); + + /* src0/src1 are not relevant to the SEL instructions, so they are filled + * with garbage. + */ + bld.BFREV(retype(src0, BRW_TYPE_UD), brw_imm_ud(1)); + bld.BFREV(retype(src1, BRW_TYPE_UD), brw_imm_ud(2)); + + bld.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src0, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + /* Unlike in sel_f_integer_negation, the types of the SEL instructions + * cannot be changed to D because both SEL instructions write the + * accumulator. Integer accumulator and float accumulator do not map the + * bits the same way, so the types cannot be changed. + */ + bld.SEL(acc0, + /* The type cast here enables using values that are integer negations + * of each other. + */ + retype(brw_imm_d(1), BRW_TYPE_F), + brw_imm_f(2.0)) + ->predicate = BRW_PREDICATE_NORMAL; + + bld.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src1, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + bld.SEL(acc0, + /* The type cast here enables using values that are integer negations + * of each other. + */ + retype(brw_imm_d(-1), BRW_TYPE_F), + brw_imm_f(3.0)) + ->predicate = BRW_PREDICATE_NORMAL; + + EXPECT_PROGRESS(brw_opt_combine_constants, bld); + + exp.BFREV(retype(src0, BRW_TYPE_UD), brw_imm_ud(1)); + exp.BFREV(retype(src1, BRW_TYPE_UD), brw_imm_ud(2)); + + exp.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src0, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + tmp1.stride = 0; + exp.uniform().MOV(tmp1, brw_imm_d(1)); + exp.SEL(acc0, + retype(tmp1, BRW_TYPE_F), + brw_imm_f(2.0)) + ->predicate = BRW_PREDICATE_NORMAL; + + exp.CMP(retype(brw_null_reg(), BRW_TYPE_F), + src1, brw_imm_f(0.0), BRW_CONDITIONAL_Z); + + tmp1.offset = 4; + exp.uniform().MOV(tmp1, brw_imm_d(-1)); + exp.SEL(acc0, + retype(tmp1, BRW_TYPE_F), + brw_imm_f(3.0)) + ->predicate = BRW_PREDICATE_NORMAL; + + EXPECT_SHADERS_MATCH(bld, exp); +}