[Mesa-stable] [PATCH] i965/vec4: Fix null destination register in 3-source instructions

[Tapani Pälli]

Yes, this fixes the app where I saw this issue, thanks!

Tested-by: Tapani Pälli <tapani.palli at intel.com>

On 03/23/2018 10:54 PM, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at 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.romanick at intel.com>
> Cc: Tapani Pälli <tapani.palli at intel.com>
> Cc: Matt Turner <mattst88 at gmail.com>
> Cc: mesa-stable at lists.freedesktop.org
> Fixes: ee63933a7 ("nir: Distribute binary operations with constants into bcsel")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105704
> ---
>   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();
>