[Mesa-dev] [PATCH] tgsi: document and fixup IF and BREAKC instrunctions

[Roland Scheidegger]

Am 11.04.2013 19:09, schrieb Zack Rusin:
> ----- Original Message -----
>> Am 11.04.2013 05:20, schrieb Zack Rusin:
>>> As implemented in tgsi_exec they both test src operands on the bit
>>> level and don't do floating point comperisons as some thought.
>>> This documents them as such to avoid future confusion and fixes
>>> their implementation in llvmpipe.
>>>
>>> Could gallium driver developers make sure that they're handling
>>> those instrunctions correctly? From my quick glance it seems
>>> to be ok, but I don't know those codebases at all.
>>>
>>> Signed-off-by: Zack Rusin <zackr at vmware.com>
>>> ---
>>>  src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c |   10 ++++------
>>>  src/gallium/auxiliary/tgsi/tgsi_info.c          |    2 ++
>>>  src/gallium/docs/source/tgsi.rst                |   16 ++++++++++------
>>>  3 files changed, 16 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>>> b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>>> index 853de09..8a29635 100644
>>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>>> @@ -2370,12 +2370,9 @@ breakc_emit(
>>>     struct lp_build_emit_data * emit_data)
>>>  {
>>>     struct lp_build_tgsi_soa_context * bld = lp_soa_context(bld_base);
>>> -   LLVMBuilderRef builder = bld_base->base.gallivm->builder;
>>>     struct lp_build_context *uint_bld = &bld_base->uint_bld;
>>> -   LLVMValueRef unsigned_cond =
>>> -      LLVMBuildBitCast(builder, emit_data->args[0], uint_bld->vec_type,
>>> "");
>>>     LLVMValueRef cond = lp_build_cmp(uint_bld, PIPE_FUNC_NOTEQUAL,
>>> -                                    unsigned_cond,
>>> +                                    emit_data->args[0],
>>>                                      uint_bld->zero);
>>>  
>>>     lp_exec_break_condition(&bld->exec_mask, cond);
>>> @@ -2389,9 +2386,10 @@ if_emit(
>>>  {
>>>     LLVMValueRef tmp;
>>>     struct lp_build_tgsi_soa_context * bld = lp_soa_context(bld_base);
>>> +   struct lp_build_context *uint_bld = &bld_base->uint_bld;
>>>  
>>> -   tmp = lp_build_cmp(&bld_base->base, PIPE_FUNC_NOTEQUAL,
>>> -                      emit_data->args[0], bld->bld_base.base.zero);
>>> +   tmp = lp_build_cmp(uint_bld, PIPE_FUNC_NOTEQUAL,
>>> +                      emit_data->args[0], uint_bld->zero);
>>>     lp_exec_mask_cond_push(&bld->exec_mask, tmp);
>>>  }
>>>  
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> index 1fadfec..f488351 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>> @@ -297,6 +297,8 @@ tgsi_opcode_infer_src_type( uint opcode )
>>>     case TGSI_OPCODE_TXF:
>>>     case TGSI_OPCODE_SAMPLE_I:
>>>     case TGSI_OPCODE_SAMPLE_I_MS:
>>> +   case TGSI_OPCODE_IF:
>>> +   case TGSI_OPCODE_BREAKC:
>>>        return TGSI_TYPE_UNSIGNED;
>>>     case TGSI_OPCODE_MOD:
>>>     case TGSI_OPCODE_I2F:
>>> diff --git a/src/gallium/docs/source/tgsi.rst
>>> b/src/gallium/docs/source/tgsi.rst
>>> index 28308cb..e6d97d6 100644
>>> --- a/src/gallium/docs/source/tgsi.rst
>>> +++ b/src/gallium/docs/source/tgsi.rst
>>> @@ -863,10 +863,19 @@ This instruction replicates its result.
>>>  
>>>    TBD
>>>  
>>> +
>>> +.. opcode:: BREAKC - Break Conditional
>>> +
>>> +  Conditionally moves the point of execution to the instruction after the
>>> +  next endloop or endswitch. The instruction must appear within a
>>> loop/endloop
>>> +  or switch/endswitch. The 32-bit register supplied by src0 is tested at a
>>> +  bit level (treat it as unsigned).
>>> +
>>>  
>>>  .. opcode:: IF - If
>>>  
>>> -  TBD
>>> +  Branch based on logical OR result. The 32-bit register supplied by src0
>>> is
>>> +  tested at a bit level. If any bit is nonzero, it will evaluate to true.
>> Maybe something similar to the "If any bit is nonzero, ..." part could
>> be added to the breakc comment as well?
>> Also I'm not convinced that "Branch based on logical OR result" makes
>> things really clearer.
> 
> Good point. How about:
> 
> .. opcode:: BREAKC - Break Conditional
> 
>   Conditionally moves the point of execution to the instruction after the
>   next endloop or endswitch. The instruction must appear within a loop/endloop
>   or switch/endswitch. The 32-bit register supplied by src0 is tested at a
>   bit level (treat it as unsigned). If any bit is nonzero, it will evaluate
>   to true.
> 
> 
> .. opcode:: IF - If
> 
>   Branch if the condition evaluates to true. The 32-bit register supplied by
>   src0 is tested at a bit level. If any bit is nonzero, it will evaluate
>   to true.
> 
> ?
> 

Looks perfect to me.

Roland