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

[Tapani Pälli]

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.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
> 
> 
> 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.


> 
> 	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();