[Mesa-dev] [PATCH 3/5] i965/fs: Add support for textureGather(.., comp)

[Chris Forbes]

I'm not sure about the compile-time error, or how to hack it in -- at
ast_to_hir time, function calls haven't been inlined yet, so I think
this gets really nasty?

Will address it in a follow-up patch though as you suggest.

-- Chris

On Sun, Oct 6, 2013 at 7:06 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 10/05/2013 03:38 AM, Chris Forbes wrote:
>> - For HSW: Select the channel based on the component selected (swizzle
>>   is done in HW)
>> - For IVB: Select the channel based on the swizzle state for the
>>   component selected. Only apply the RG32F w/a if we actually want
>>   green -- we're about to flag it regardless of swizzle state.
>>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> index 6141009..5508cdc 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> @@ -1475,7 +1475,8 @@ fs_visitor::visit(ir_texture *ir)
>>        /* When tg4 is used with the degenerate ZERO/ONE swizzles, don't bother
>>         * emitting anything other than setting up the constant result.
>>         */
>> -      int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0);
>> +      ir_constant *chan = ir->lod_info.component->as_constant();
>> +      int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0]);
>
> Cool!  On my first pass through the spec I hadn't realized that the
> channel parameter was required to be a constant integer expression, and
> you indeed enforced that by using ir_var_const_in.  Makes life here so
> much easier.
>
> There's only one problem: right now, nothing enforces the constraint
> that the component parameter must be 0, 1, 2, or 3.  Although it isn't
> explicitly stated in the spec, I believe specifying another value should
> be a compile error.  I think this has to get handled at the ast_to_hir
> step, and can probably be an ugly special case for now.
>
> It looks like currently, the bitshifts in GET_SWZ will probably return
> 0, resulting in the red channel being selected.  So there isn't an out
> of bounds access problem...just undefined results.  A compile error is
> definitely more friendly, though.
>
>
>>        if (swiz == SWIZZLE_ZERO || swiz == SWIZZLE_ONE) {
>>
>>           fs_reg res = fs_reg(this, glsl_type::vec4_type);
>> @@ -1594,14 +1595,16 @@ fs_visitor::visit(ir_texture *ir)
>>  uint32_t
>>  fs_visitor::gather_channel(ir_texture *ir, int sampler)
>>  {
>> -   int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0 /* red */);
>> -   if (c->key.tex.gather_channel_quirk_mask & (1<<sampler))
>> -      return 2;   /* gather4 sampler is broken for green channel on RG32F --
>> -                   * we must ask for blue instead.
>> -                   */
>> +   ir_constant *chan = ir->lod_info.component->as_constant();
>> +   int swiz = GET_SWZ(c->key.tex.swizzles[sampler], chan->value.i[0] /* red */);
>
> The /* red */ comment here doesn't make sense anymore...I'd drop it.
>
>>     switch (swiz) {
>>        case SWIZZLE_X: return 0;
>> -      case SWIZZLE_Y: return 1;
>> +      case SWIZZLE_Y:
>> +         if (c->key.tex.gather_channel_quirk_mask & (1<<sampler))
>> +            return 2;   /* gather4 sampler is broken for green channel on RG32F --
>> +                         * we must ask for blue instead.
>> +                         */
>> +         return 1;
>
> I would like to see the comment above the block, rather than off to the
> side like this.  Ditto for the next patch.
>
>>        case SWIZZLE_Z: return 2;
>>        case SWIZZLE_W: return 3;
>>        default:
>>
>
> Patches 3-4 are:
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> (since my comment about bounds checking 'comp' would go in a different
> patch anyway)