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

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

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.


  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.
+ */
+   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();
@@ -2696,6 +2720,8 @@ vec4_visitor::run()
+ 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

Reply via email to