From b23432c540b1274904d80b90eb67d1798b11d2c8 Mon Sep 17 00:00:00 2001 From: Ian Romanick Date: Tue, 17 Aug 2021 17:15:03 -0700 Subject: [PATCH] intel/fs: Fix a cmod prop bug when the source type of a mov doesn't match the dest type of scan_inst We were previously operating with the mindset "a MOV is just a compare with zero." As a result, we were trying to share as much code between the MOV path and the CMP path as possible. However, MOV instructions can perform type conversions that affect the result of the comparison. There was some code added to better handle this for cases like and(16) g31<1>UD g20<8,8,1>UD g22<8,8,1>UD mov.nz.f0(16) null<1>F g31<8,8,1>D The flaw in these changed special cases is that it allowed things like or(8) dest:D src0:D src1:D mov.nz(8) null:D dest:F Because both destinations were integer types, the propagation was allowed. The source type of the MOV and the destination type of the OR do not match, so type conversion rules have to be accounted for. My solution was to just split the MOV and non-MOV paths with completely separate checks. The "else" path in this commit is basically the old code with the BRW_OPCODE_MOV special case removed. The new MOV code further splits into "destination of scan_inst is float" and "destination of scan_inst is integer" paths. For each case I enumerate the rules that I belive apply. For the integer path, only the "Z or NZ" rules are listed as only NZ is currently allowed (hence the conditional_mod assertion in that path). A later commit relaxes this and adds the rule. The new rules slightly relax one of the previous rules. Previously the sizes of the MOV destination and the MOV source had to be the same. In some cases now the sizes can be different by the following conditions: - Floating point to integer conversion are not allowed. - If the conversion is integer to floating point, the size of the floating point value does not matter as it will not affect the comparison result. - If the conversion is float to float, the size of the destination must be greater than or equal to the size of the source. - If the conversion is integer to integer, the size of the destination must be greater than or equal to the size of the source. Reviewed-by: Kenneth Graunke Part-of: --- .../compiler/brw_fs_cmod_propagation.cpp | 89 +++++++++++++------ .../compiler/test_fs_cmod_propagation.cpp | 43 +++++++++ 2 files changed, 103 insertions(+), 29 deletions(-) diff --git a/src/intel/compiler/brw_fs_cmod_propagation.cpp b/src/intel/compiler/brw_fs_cmod_propagation.cpp index c3c59dc1b05..5f30c3b1d23 100644 --- a/src/intel/compiler/brw_fs_cmod_propagation.cpp +++ b/src/intel/compiler/brw_fs_cmod_propagation.cpp @@ -320,37 +320,68 @@ opt_cmod_propagation_local(const intel_device_info *devinfo, bblock_t *block) if (inst->opcode == BRW_OPCODE_AND) break; - /* Not safe to use inequality operators if the types are different - */ - if (scan_inst->dst.type != inst->src[0].type && - inst->conditional_mod != BRW_CONDITIONAL_Z && - inst->conditional_mod != BRW_CONDITIONAL_NZ) - break; - - /* Comparisons operate differently for ints and floats */ - if (scan_inst->dst.type != inst->dst.type) { - /* Comparison result may be altered if the bit-size changes - * since that affects range, denorms, etc - */ - if (type_sz(scan_inst->dst.type) != type_sz(inst->dst.type)) - break; - - /* We should propagate from a MOV to another instruction in a - * sequence like: - * - * and(16) g31<1>UD g20<8,8,1>UD g22<8,8,1>UD - * mov.nz.f0(16) null<1>F g31<8,8,1>D - */ - if (inst->opcode == BRW_OPCODE_MOV) { - if ((inst->src[0].type != BRW_REGISTER_TYPE_D && - inst->src[0].type != BRW_REGISTER_TYPE_UD) || - (scan_inst->dst.type != BRW_REGISTER_TYPE_D && - scan_inst->dst.type != BRW_REGISTER_TYPE_UD)) { + if (inst->opcode == BRW_OPCODE_MOV) { + if (brw_reg_type_is_floating_point(scan_inst->dst.type)) { + /* If the destination type of scan_inst is floating-point, + * then: + * + * - The source of the MOV instruction must be the same + * type. + * + * - The destination of the MOV instruction must be float + * point with a size at least as large as the destination + * of inst. Size-reducing f2f conversions could cause + * non-zero values to become zero, etc. + */ + if (scan_inst->dst.type != inst->src[0].type) break; - } - } else if (brw_reg_type_is_floating_point(scan_inst->dst.type) != - brw_reg_type_is_floating_point(inst->dst.type)) { + + if (!brw_reg_type_is_floating_point(inst->dst.type)) + break; + + if (type_sz(scan_inst->dst.type) > type_sz(inst->dst.type)) + break; + } else { + /* If the destination type of scan_inst is integer, then: + * + * - The source of the MOV instruction must be integer with + * the same size. + * + * - 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 + * as the destination of inst. + */ + if (!brw_reg_type_is_integer(inst->src[0].type) || + type_sz(scan_inst->dst.type) != type_sz(inst->src[0].type)) + break; + + assert(inst->conditional_mod == BRW_CONDITIONAL_NZ); + + if (brw_reg_type_is_integer(inst->dst.type) && + type_sz(inst->dst.type) < type_sz(scan_inst->dst.type)) + break; + } + } else { + /* Not safe to use inequality operators if the types are + * different. + */ + if (scan_inst->dst.type != inst->src[0].type && + inst->conditional_mod != BRW_CONDITIONAL_Z && + inst->conditional_mod != BRW_CONDITIONAL_NZ) break; + + /* Comparisons operate differently for ints and floats */ + if (scan_inst->dst.type != inst->dst.type) { + /* Comparison result may be altered if the bit-size changes + * since that affects range, denorms, etc + */ + if (type_sz(scan_inst->dst.type) != type_sz(inst->dst.type)) + break; + + if (brw_reg_type_is_floating_point(scan_inst->dst.type) != + brw_reg_type_is_floating_point(inst->dst.type)) + break; } } diff --git a/src/intel/compiler/test_fs_cmod_propagation.cpp b/src/intel/compiler/test_fs_cmod_propagation.cpp index 4c58684aa3f..f3cb4c4ebd3 100644 --- a/src/intel/compiler/test_fs_cmod_propagation.cpp +++ b/src/intel/compiler/test_fs_cmod_propagation.cpp @@ -1505,6 +1505,49 @@ TEST_F(cmod_propagation_test, signed_unsigned_comparison_mismatch) EXPECT_EQ(BRW_CONDITIONAL_LE, instruction(block0, 1)->conditional_mod); } +TEST_F(cmod_propagation_test, ior_f2i_nz) +{ + const fs_builder &bld = v->bld; + fs_reg dest = bld.vgrf(BRW_REGISTER_TYPE_D); + fs_reg src0 = bld.vgrf(BRW_REGISTER_TYPE_D); + fs_reg src1 = bld.vgrf(BRW_REGISTER_TYPE_D); + + bld.OR(dest, src0, src1); + bld.MOV(bld.null_reg_d(), retype(dest, BRW_REGISTER_TYPE_F)) + ->conditional_mod = BRW_CONDITIONAL_NZ; + + /* = Before = + * 0: or(8) dest:D src0:D src1:D + * 1: mov.nz(8) null:D dest:F + * + * = After = + * No changes. + * + * If src0 = 0x30000000 and src1 = 0x0f000000, then the value stored in + * dest, interpreted as floating point, is 0.5. This bit pattern is not + * zero, but after the float-to-integer conversion, the value is zero. + */ + v->calculate_cfg(); + 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_OR, 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_NZ, instruction(block0, 1)->conditional_mod); +} + + void cmod_propagation_test::test_mov_prop(enum brw_conditional_mod cmod, enum brw_reg_type add_type,