From fa51595c7f1fb0311371a256cc0a5c1cc2e11d9a Mon Sep 17 00:00:00 2001 From: Sviatoslav Peleshko Date: Tue, 3 Sep 2024 14:48:07 +0300 Subject: [PATCH] brw: Fix mov cmod propagation when there's int signedness mismatch If there's difference between scan_inst dest type and inst src type we should be more careful, because difference in signedness can cause incorrect results after the propagation. Updated ror-default.trace hash, as the change fixes misrendering there. Fixes: b23432c5 ("intel/fs: Fix a cmod prop bug when the source type of a mov doesn't match the dest type of scan_inst") Signed-off-by: Sviatoslav Peleshko Reviewed-by: Ian Romanick Reviewed-by: Lionel Landwerlin Part-of: --- src/intel/ci/traces-iris.yml | 12 +++--- .../compiler/brw_fs_cmod_propagation.cpp | 10 +++++ .../compiler/test_fs_cmod_propagation.cpp | 43 +++++++++++++++++++ 3 files changed, 59 insertions(+), 6 deletions(-) diff --git a/src/intel/ci/traces-iris.yml b/src/intel/ci/traces-iris.yml index b5340835fed..9d0e46adf07 100644 --- a/src/intel/ci/traces-iris.yml +++ b/src/intel/ci/traces-iris.yml @@ -395,17 +395,17 @@ traces: checksum: 757e0c9a37ecfc5a2efb10505f98ad95 ror/ror-default.trace: gl-intel-apl: - checksum: e604a0d1ecea40e53431e0aa5e6c8276 + checksum: dde3a4118bca61afdb5fcd94c30e61cd gl-intel-glk: - checksum: e604a0d1ecea40e53431e0aa5e6c8276 + checksum: dde3a4118bca61afdb5fcd94c30e61cd gl-intel-amly: - checksum: e604a0d1ecea40e53431e0aa5e6c8276 + checksum: dde3a4118bca61afdb5fcd94c30e61cd gl-intel-kbl: - checksum: e604a0d1ecea40e53431e0aa5e6c8276 + checksum: dde3a4118bca61afdb5fcd94c30e61cd gl-intel-whl: - checksum: e604a0d1ecea40e53431e0aa5e6c8276 + checksum: dde3a4118bca61afdb5fcd94c30e61cd gl-intel-cml: - checksum: e604a0d1ecea40e53431e0aa5e6c8276 + checksum: dde3a4118bca61afdb5fcd94c30e61cd valve/counterstrike-source-v2.trace: gl-intel-apl: checksum: 838ed7482efd5e9d7bde2b67e62314c4 diff --git a/src/intel/compiler/brw_fs_cmod_propagation.cpp b/src/intel/compiler/brw_fs_cmod_propagation.cpp index 17ef890dca4..703624670df 100644 --- a/src/intel/compiler/brw_fs_cmod_propagation.cpp +++ b/src/intel/compiler/brw_fs_cmod_propagation.cpp @@ -353,6 +353,10 @@ opt_cmod_propagation_local(const intel_device_info *devinfo, bblock_t *block) * - The source of the MOV instruction must be integer with * the same size. * + * - If the conditional modifier is neither Z nor NZ, then + * the source of the MOV instruction has to have same + * signedness. + * * - If the conditional modifier is Z or NZ, then the * destination type of inst must either be floating point * (of any size) or integer with a size at least as large @@ -367,6 +371,12 @@ opt_cmod_propagation_local(const intel_device_info *devinfo, bblock_t *block) brw_type_size_bits(scan_inst->dst.type) != brw_type_size_bits(inst->src[0].type)) break; + if (inst->conditional_mod != BRW_CONDITIONAL_Z && + inst->conditional_mod != BRW_CONDITIONAL_NZ && + brw_type_is_uint(inst->src[0].type) != + brw_type_is_uint(scan_inst->dst.type)) + break; + if (brw_type_is_int(inst->dst.type)) { if (brw_type_size_bits(inst->dst.type) < brw_type_size_bits(scan_inst->dst.type)) diff --git a/src/intel/compiler/test_fs_cmod_propagation.cpp b/src/intel/compiler/test_fs_cmod_propagation.cpp index 6d5b495563e..325a36702d2 100644 --- a/src/intel/compiler/test_fs_cmod_propagation.cpp +++ b/src/intel/compiler/test_fs_cmod_propagation.cpp @@ -1542,6 +1542,49 @@ TEST_F(cmod_propagation_test, ior_f2i_nz) EXPECT_EQ(BRW_CONDITIONAL_NZ, instruction(block0, 1)->conditional_mod); } +TEST_F(cmod_propagation_test, uand_b2f_g) +{ + brw_reg dest = bld.vgrf(BRW_TYPE_UD); + brw_reg src0 = bld.vgrf(BRW_TYPE_UD); + brw_reg src1 = bld.vgrf(BRW_TYPE_UD); + + bld.AND(dest, src0, src1); + bld.MOV(bld.null_reg_f(), negate(retype(dest, BRW_TYPE_D))) + ->conditional_mod = BRW_CONDITIONAL_G; + + /* = Before = + * 0: and(8) dest:UD src0:UD src1:UD + * 1: mov.g(8) null:F -dest:D + * + * = After = + * No changes. + * + * If src0 and src1 are 0xffffffff, then dest:D will be interpreted as -1, + * and -dest:D will be 1, which is > 0. + * If the cmod was propagated (and.l(8) dest:UD src0:UD src1:UD), + * dest:UD can never be < 0. + * + */ + brw_calculate_cfg(*v); + bblock_t *block0 = v->cfg->blocks[0]; + + EXPECT_EQ(0, block0->start_ip); + EXPECT_EQ(1, block0->end_ip); + + EXPECT_FALSE(cmod_propagation(v)); + EXPECT_EQ(0, block0->start_ip); + + EXPECT_EQ(BRW_OPCODE_AND, instruction(block0, 0)->opcode); + EXPECT_EQ(BRW_CONDITIONAL_NONE, instruction(block0, 0)->conditional_mod); + + /* This is ASSERT_EQ because if end_ip is 0, the instruction(block0, 1) + * calls will not work properly, and the test will give weird results. + */ + ASSERT_EQ(1, block0->end_ip); + EXPECT_EQ(BRW_OPCODE_MOV, instruction(block0, 1)->opcode); + EXPECT_EQ(BRW_CONDITIONAL_G, instruction(block0, 1)->conditional_mod); + EXPECT_TRUE(instruction(block0, 1)->src[0].negate); +} void cmod_propagation_test::test_mov_prop(enum brw_conditional_mod cmod,