Mesa (main): intel/fs: Fix a cmod prop bug when the source type of a mov doesn't match the dest type of scan_inst
GitLab Mirror
gitlab-mirror at kemper.freedesktop.org
Mon Aug 30 21:11:44 UTC 2021
Module: Mesa
Branch: main
Commit: b23432c540b1274904d80b90eb67d1798b11d2c8
URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=b23432c540b1274904d80b90eb67d1798b11d2c8
Author: Ian Romanick <ian.d.romanick at intel.com>
Date: Tue Aug 17 17:15:03 2021 -0700
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 <kenneth at whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12045>
---
src/intel/compiler/brw_fs_cmod_propagation.cpp | 83 +++++++++++++++++--------
src/intel/compiler/test_fs_cmod_propagation.cpp | 43 +++++++++++++
2 files changed, 100 insertions(+), 26 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;
+ 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;
+
+ 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;
- /* 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
+ 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 (type_sz(scan_inst->dst.type) != type_sz(inst->dst.type))
+ if (scan_inst->dst.type != inst->src[0].type &&
+ inst->conditional_mod != BRW_CONDITIONAL_Z &&
+ inst->conditional_mod != BRW_CONDITIONAL_NZ)
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)) {
+ /* 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;
- }
- } else 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,
More information about the mesa-commit
mailing list