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

[Tapani Pälli]

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

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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev