On 04/11/2018 11:02 AM, Tapani Pälli wrote:

On 04/11/2018 10:42 AM, Juan A. Suarez Romero wrote:
On Fri, 2018-03-23 at 13:54 -0700, Ian Romanick wrote:
From: Ian Romanick <ian.d.roman...@intel.com>

A recent commit (see below) triggered some cases where conditional
modifier propagation and dead code elimination would cause a MAD
instruction like the following to be generated:

     mad.l.f0  null, ...

Matt pointed out that fs_visitor::fixup_3src_null_dest() fixes cases
like this in the scalar backend.  This commit basically ports that code
to the vec4 backend.

NOTE: I have sent a couple tests to the piglit list that reproduce this
bug *without* the commit mentioned below.  This commit fixes those
tests.

Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
Cc: Tapani Pälli <tapani.pa...@intel.com>
Cc: Matt Turner <matts...@gmail.com>
Cc: mesa-sta...@lists.freedesktop.org
Fixes: ee63933a7 ("nir: Distribute binary operations with constants into bcsel")
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105704


I need a clarification with this commit for inclusion on next 18.0 stable
release.

The commit was nominated for stable, but at the same time it has a "Fixes" tag.

The commit applies cleanly in the stable branch, but the commit it fixes
(ee63933a7) was not landed in branch.


So I'd like to know if this patch should be included in stable or not.

Yes it should, it will make some Piglit tests pass (see Piglit 5194fc31b). It fixes existing issue that got exposed by Mesa commit ee63933a7.

What I meant to say is 'more exposed', the issue is already triggered without that Mesa commit so this patch fixes existing issue.



    J.A.



---
  src/intel/compiler/brw_vec4.cpp | 26 ++++++++++++++++++++++++++
  src/intel/compiler/brw_vec4.h   |  1 +
  2 files changed, 27 insertions(+)

diff --git a/src/intel/compiler/brw_vec4.cpp b/src/intel/compiler/brw_vec4.cpp
index e483814..fb8ffee 100644
--- a/src/intel/compiler/brw_vec4.cpp
+++ b/src/intel/compiler/brw_vec4.cpp
@@ -1945,6 +1945,30 @@ is_align1_df(vec4_instruction *inst)
     }
  }
+/**
+ * Three source instruction must have a GRF/MRF destination register.
+ * ARF NULL is not allowed.  Fix that up by allocating a temporary GRF.
+ */
+void
+vec4_visitor::fixup_3src_null_dest()
+{
+   bool progress = false;
+
+   foreach_block_and_inst_safe (block, vec4_instruction, inst, cfg) {
+      if (inst->is_3src(devinfo) && inst->dst.is_null()) {
+         const unsigned size_written = type_sz(inst->dst.type);
+         const unsigned num_regs = DIV_ROUND_UP(size_written, REG_SIZE);
+
+         inst->dst = retype(dst_reg(VGRF, alloc.allocate(num_regs)),
+                            inst->dst.type);
+         progress = true;
+      }
+   }
+
+   if (progress)
+      invalidate_live_intervals();
+}
+
  void
  vec4_visitor::convert_to_hw_regs()
  {
@@ -2696,6 +2720,8 @@ vec4_visitor::run()
        OPT(scalarize_df);
     }
+   fixup_3src_null_dest();
+
     bool allocated_without_spills = reg_allocate();
     if (!allocated_without_spills) {
diff --git a/src/intel/compiler/brw_vec4.h b/src/intel/compiler/brw_vec4.h
index 39ce51c..71880db 100644
--- a/src/intel/compiler/brw_vec4.h
+++ b/src/intel/compiler/brw_vec4.h
@@ -158,6 +158,7 @@ public:
     void opt_set_dependency_control();
     void opt_schedule_instructions();
     void convert_to_hw_regs();
+   void fixup_3src_null_dest();
     bool is_supported_64bit_region(vec4_instruction *inst, unsigned arg);
     bool lower_simd_width();
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to