[Mesa-dev] [PATCH 09/12] i965/fs: Hook up SIMD lowering to handle texturing opcodes of unsupported width.

[Jason Ekstrand]

On Wed, Jul 22, 2015 at 12:55 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> Jason Ekstrand <jason at jlekstrand.net> writes:
>
>> On Sat, Jul 18, 2015 at 7:34 AM, Francisco Jerez <currojerez at riseup.net> wrote:
>>> This should match the set of cases in which we currently call fail()
>>> or no16() from the emit_texture_*() methods and the ones in which
>>> emit_texture_gen4() enables the SIMD16 workaround.
>>>
>>> Hint for reviewers: It's not a big deal if I happen to have missed
>>> some case here, it will just lead to an assertion failure down the
>>> road which is easily fixable, however being stricter than necessary
>>> won't cause any visible breakage, it would just decrease performance
>>> silently due to the unnecessary message splitting, so feel free to
>>> double-check that all cases listed here already cause a SIMD8/16
>>> fall-back with the current texturing code -- You may want to skip over
>>> the Gen5-6 cases though if you don't have pencil and paper at hand.
>>
>> I'll believe you.  Even if you got it wrong, the worst that happens is
>> we hit the fail case when we try and lower.
>>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_fs.cpp | 27 +++++++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> index 043d9e9..f291202 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> @@ -3918,6 +3918,33 @@ get_lowered_simd_width(const struct brw_device_info *devinfo,
>>>        /* Dual-source FB writes are unsupported in SIMD16 mode. */
>>>        return (inst->src[1].file != BAD_FILE ? 8 : inst->exec_size);
>>>
>>> +   case SHADER_OPCODE_TXD_LOGICAL:
>>> +      /* TXD is unsupported in SIMD16 mode. */
>>> +      return 8;
>>> +
>>> +   case SHADER_OPCODE_TG4_OFFSET_LOGICAL: {
>>> +      /* gather4_po_c is unsupported in SIMD16 mode. */
>>> +      const fs_reg &shadow_c = inst->src[1];
>>> +      return (shadow_c.file != BAD_FILE ? 8 : inst->exec_size);
>>> +   }
>>> +   case SHADER_OPCODE_TXL_LOGICAL:
>>> +   case FS_OPCODE_TXB_LOGICAL: {
>>> +      /* Gen4 doesn't have SIMD8 non-shadow-compare bias/LOD instructions, and
>>> +       * Gen4-6 don't support TXL and TXB with shadow comparison in SIMD16
>>> +       * mode.
>>> +       */
>>> +      const fs_reg &shadow_c = inst->src[1];
>>> +      return (devinfo->gen == 4 && shadow_c.file == BAD_FILE ? 16 :
>>> +              devinfo->gen < 7 && shadow_c.file != BAD_FILE ? 8 :
>>> +              inst->exec_size);
>>
>> Could we please use an if-ladder here.  Nested ternaries are hard to
>> read and kind of pointless given that we can return multiple places in
>> the if.
>>
> Heh, I always have the very opposite style itch -- If you can compute
> something succinctly with a handful of "pure" expressions why venture
> into the dangerous realm of statements?

Heh... The mathematician in me is tempted to agree with you.  However,
the programmer in me likes his control-flow to have obvious nesting
and indentation. :-)

> I think John Backus' article "Can programming be liberated from the von
> Neumann style?" although slightly outdated elaborates the point pretty
> well.
>
> But sure, if you'd like it better with an if-ladder why not.
>
>>> +   }
>>> +   case SHADER_OPCODE_TXF_LOGICAL:
>>> +   case SHADER_OPCODE_TXS_LOGICAL:
>>> +      /* Gen4 doesn't have SIMD8 variants for the RESINFO and LD-with-LOD
>>> +       * messages.  Use SIMD16 instead.
>>> +       */
>>> +      return (devinfo->gen == 4 ? 16 : inst->exec_size);
>>
>> I'd kind of also like this ternary to go, but I'm ok with it if you
>> want to keep it.
>>
>>> +
>>>     default:
>>>        return inst->exec_size;
>>>     }
>>> --
>>> 2.4.3
>>>