intel/fs: Fix a cmod prop bug when the source type of a mov doesn't match the dest...

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@whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/12045>

diff --git a/src/intel/compiler/brw_fs_cmod_propagation.cpp b/src/intel/compiler/brw_fs_cmod_propagation.cpp
index c3c59dc..5f30c3b 100644 (file)
--- 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 4c58684..f3cb4c4 100644 (file)
--- 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,