[Mesa-dev] [PATCH] gallium: remove support for predicates from TGSI

[Jose Fonseca]

On 29/03/17 19:02, Roland Scheidegger wrote:
> [resend with snipped bits as it's too big]
>
> A couple comments inline.
>
> [snip]
>
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> @@ -746,39 +746,30 @@ static void lp_exec_default(struct lp_exec_mask *mask,
>>  }
>>
>>
>>  /* stores val into an address pointed to by dst_ptr.
>>   * mask->exec_mask is used to figure out which bits of val
>>   * should be stored into the address
>>   * (0 means don't store this bit, 1 means do store).
>>   */
>>  static void lp_exec_mask_store(struct lp_exec_mask *mask,
>>                                 struct lp_build_context *bld_store,
>> -                               LLVMValueRef pred,
>>                                 LLVMValueRef val,
>>                                 LLVMValueRef dst_ptr)
>>  {
>>     LLVMBuilderRef builder = mask->bld->gallivm->builder;
>> +   LLVMValueRef pred = mask->has_mask ? mask->exec_mask : NULL;
> Calling this "pred" now seems to be somewhat of a misnomer (wasn't all
> that great before because it then included exec_mask but it's worse now).
>
>
>
>>
>>     assert(lp_check_value(bld_store->type, val));
>>     assert(LLVMGetTypeKind(LLVMTypeOf(dst_ptr)) == LLVMPointerTypeKind);
>>     assert(LLVMGetElementType(LLVMTypeOf(dst_ptr)) == LLVMTypeOf(val));
>>
>> -   /* Mix the predicate and execution mask */
>> -   if (mask->has_mask) {
>> -      if (pred) {
>> -         pred = LLVMBuildAnd(builder, pred, mask->exec_mask, "");
>> -      } else {
>> -         pred = mask->exec_mask;
>> -      }
>> -   }
>> -
>>     if (pred) {
>>        LLVMValueRef res, dst;
>>
>>        dst = LLVMBuildLoad(builder, dst_ptr, "");
>>        res = lp_build_select(bld_store, pred, val, dst);
>>        LLVMBuildStore(builder, res, dst_ptr);
>>     } else
>>        LLVMBuildStore(builder, val, dst_ptr);
>>  }
>>
>> @@ -1029,36 +1020,26 @@ build_gather(struct lp_build_tgsi_context *bld_base,
>>
>>
>>  /**
>>   * Scatter/store vector.
>>   */
>>  static void
>>  emit_mask_scatter(struct lp_build_tgsi_soa_context *bld,
>>                    LLVMValueRef base_ptr,
>>                    LLVMValueRef indexes,
>>                    LLVMValueRef values,
>> -                  struct lp_exec_mask *mask,
>> -                  LLVMValueRef pred)
>> +                  struct lp_exec_mask *mask)
>>  {
>>     struct gallivm_state *gallivm = bld->bld_base.base.gallivm;
>>     LLVMBuilderRef builder = gallivm->builder;
>>     unsigned i;
>> -
>> -   /* Mix the predicate and execution mask */
>> -   if (mask->has_mask) {
>> -      if (pred) {
>> -         pred = LLVMBuildAnd(builder, pred, mask->exec_mask, "");
>> -      }
>> -      else {
>> -         pred = mask->exec_mask;
>> -      }
>> -   }
>> +   LLVMValueRef pred = mask->has_mask ? mask->exec_mask : NULL;
> same here.
>
>
>> diff --git a/src/gallium/include/pipe/p_shader_tokens.h b/src/gallium/include/pipe/p_shader_tokens.h
>> index 6a3fb98..87d2d92 100644
>> --- a/src/gallium/include/pipe/p_shader_tokens.h
>> +++ b/src/gallium/include/pipe/p_shader_tokens.h
>> @@ -62,21 +62,20 @@ struct tgsi_token
>>
>>  enum tgsi_file_type {
>>     TGSI_FILE_NULL,
>>     TGSI_FILE_CONSTANT,
>>     TGSI_FILE_INPUT,
>>     TGSI_FILE_OUTPUT,
>>     TGSI_FILE_TEMPORARY,
>>     TGSI_FILE_SAMPLER,
>>     TGSI_FILE_ADDRESS,
>>     TGSI_FILE_IMMEDIATE,
>> -   TGSI_FILE_PREDICATE,
>>     TGSI_FILE_SYSTEM_VALUE,
>>     TGSI_FILE_IMAGE,
>>     TGSI_FILE_SAMPLER_VIEW,
>>     TGSI_FILE_BUFFER,
>>     TGSI_FILE_MEMORY,
>>     TGSI_FILE_COUNT,      /**< how many TGSI_FILE_ types */
>>  };
>>
>>
>>  #define TGSI_WRITEMASK_NONE     0x00
>> @@ -609,34 +608,31 @@ struct tgsi_property_data {
>>
>>  /**
>>   * Opcode is the operation code to execute. A given operation defines the
>>   * semantics how the source registers (if any) are interpreted and what is
>>   * written to the destination registers (if any) as a result of execution.
>>   *
>>   * NumDstRegs and NumSrcRegs is the number of destination and source registers,
>>   * respectively. For a given operation code, those numbers are fixed and are
>>   * present here only for convenience.
>>   *
>> - * If Predicate is TRUE, tgsi_instruction_predicate token immediately follows.
>> - *
>>   * Saturate controls how are final results in destination registers modified.
>>   */
>>
>>  struct tgsi_instruction
>>  {
>>     unsigned Type       : 4;  /* TGSI_TOKEN_TYPE_INSTRUCTION */
>>     unsigned NrTokens   : 8;  /* UINT */
>>     unsigned Opcode     : 8;  /* TGSI_OPCODE_ */
>>     unsigned Saturate   : 1;  /* BOOL */
>>     unsigned NumDstRegs : 2;  /* UINT */
>>     unsigned NumSrcRegs : 4;  /* UINT */
>> -   unsigned Predicate  : 1;  /* BOOL */
>>     unsigned Label      : 1;
>>     unsigned Texture    : 1;
>>     unsigned Memory     : 1;
>>     unsigned Padding    : 1;
> The Padding doesn't match.
>
>
>
> So, we still have code which uses this - however this code is only used
> for some testing, otherwise we translate this d3d9 stuff away like
> everybody else.
> Maybe it's time to ditch this stuff then - clearly no other drivers are
> ever going to support it and apis have moved away from it.
>
> Jose, any opinion on that?

Yes, I agree.

Jose