[Mesa-dev] [PATCH 18/22] i965/fs: Add infrastructure for generating CSEL instructions.

Ian Romanick idr at freedesktop.org
Mon Mar 5 19:17:29 UTC 2018


On 02/28/2018 12:58 AM, Samuel Iglesias Gonsálvez wrote:
> On 24/02/18 00:56, Ian Romanick wrote:
>> From: Kenneth Graunke <kenneth at whitecape.org>
>>
>> v2 (idr): Don't allow CSEL with a non-float src2.
>>
>> v3 (idr): Add CSEL to fs_inst::flags_written.  Suggested by Matt.
>>
>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>> ---
>>  src/intel/compiler/brw_eu.h             |  1 +
>>  src/intel/compiler/brw_eu_emit.c        |  1 +
>>  src/intel/compiler/brw_fs.cpp           |  1 +
>>  src/intel/compiler/brw_fs_builder.h     | 22 +++++++++++++++++++++-
>>  src/intel/compiler/brw_fs_generator.cpp |  5 +++++
>>  5 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
>> index 2d0f56f..3201624 100644
>> --- a/src/intel/compiler/brw_eu.h
>> +++ b/src/intel/compiler/brw_eu.h
>> @@ -171,6 +171,7 @@ ALU2(SHR)
>>  ALU2(SHL)
>>  ALU1(DIM)
>>  ALU2(ASR)
>> +ALU3(CSEL)
>>  ALU1(F32TO16)
>>  ALU1(F16TO32)
>>  ALU2(ADD)
>> diff --git a/src/intel/compiler/brw_eu_emit.c b/src/intel/compiler/brw_eu_emit.c
>> index c25d8d6..305a759 100644
>> --- a/src/intel/compiler/brw_eu_emit.c
>> +++ b/src/intel/compiler/brw_eu_emit.c
>> @@ -953,6 +953,7 @@ ALU2(SHR)
>>  ALU2(SHL)
>>  ALU1(DIM)
>>  ALU2(ASR)
>> +ALU3(CSEL)
>>  ALU1(FRC)
>>  ALU1(RNDD)
>>  ALU2(MAC)
>> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
>> index bed632d..accae1b 100644
>> --- a/src/intel/compiler/brw_fs.cpp
>> +++ b/src/intel/compiler/brw_fs.cpp
>> @@ -929,6 +929,7 @@ unsigned
>>  fs_inst::flags_written() const
>>  {
>>     if ((conditional_mod && (opcode != BRW_OPCODE_SEL &&
>> +                            opcode != BRW_OPCODE_CSEL &&
>>                              opcode != BRW_OPCODE_IF &&
>>                              opcode != BRW_OPCODE_WHILE)) ||
>>         opcode == FS_OPCODE_MOV_DISPATCH_TO_FLAGS) {
>> diff --git a/src/intel/compiler/brw_fs_builder.h b/src/intel/compiler/brw_fs_builder.h
>> index 87394bc..47eac42 100644
>> --- a/src/intel/compiler/brw_fs_builder.h
>> +++ b/src/intel/compiler/brw_fs_builder.h
>> @@ -458,7 +458,6 @@ namespace brw {
>>        ALU1(BFREV)
>>        ALU1(CBIT)
>>        ALU2(CMPN)
>> -      ALU3(CSEL)
>>        ALU1(DIM)
>>        ALU2(DP2)
>>        ALU2(DP3)
>> @@ -534,6 +533,27 @@ namespace brw {
>>        }
>>  
>>        /**
>> +       * CSEL: dst = src2 <op> 0.0f ? src0 : src1
>> +       */
>> +      instruction *
>> +      CSEL(const dst_reg &dst, const src_reg &src0, const src_reg &src1,
>> +           const src_reg &src2, brw_conditional_mod condition) const
>> +      {
>> +         /* CSEL only operates on floats, so we can't do integer </<=/>=/>
>> +          * comparisons.  Zero/non-zero (== and !=) comparisons almost work.
>> +          * 0x80000000 fails because it is -0.0, and -0.0 == 0.0.
>> +          */
>> +         assert(src2.type == BRW_REGISTER_TYPE_F);
>> +
>> +         return set_condmod(condition,
>> +                            emit(BRW_OPCODE_CSEL,
>> +                                 retype(dst, BRW_REGISTER_TYPE_F),
>> +                                 retype(src0, BRW_REGISTER_TYPE_F),
>> +                                 retype(src1, BRW_REGISTER_TYPE_F),
>> +                                 src2));
>> +      }
>> +
>> +      /**
>>         * Emit a linear interpolation instruction.
>>         */
>>        instruction *
>> diff --git a/src/intel/compiler/brw_fs_generator.cpp b/src/intel/compiler/brw_fs_generator.cpp
>> index cd5be05..9157367 100644
>> --- a/src/intel/compiler/brw_fs_generator.cpp
>> +++ b/src/intel/compiler/brw_fs_generator.cpp
>> @@ -1826,6 +1826,11 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>>        case BRW_OPCODE_SEL:
>>  	 brw_SEL(p, dst, src[0], src[1]);
>>  	 break;
>> +      case BRW_OPCODE_CSEL:
>> +         brw_set_default_access_mode(p, BRW_ALIGN_16);
>> +         brw_CSEL(p, dst, src[0], src[1], src[2]);
>> +         brw_set_default_access_mode(p, BRW_ALIGN_1);
> 
> Setting the access mode to ALIGN_1 is not needed, right? It will
> continue with the next instruction in the loop that sets ALIGN1 at the
> beginning of the loop.

This was in Ken's original patch, so I'm not 100% sure.  I've seen this
pattern around generating some other instructions, so my gut tells me
that it's done like this to be explicit and more defensive.

> In any case,
> 
> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> 
> Sam
> 
> 
>> +         break;
>>        case BRW_OPCODE_BFREV:
>>           assert(devinfo->gen >= 7);
>>           brw_BFREV(p, retype(dst, BRW_REGISTER_TYPE_UD),
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180305/3d6837d8/attachment.sig>


More information about the mesa-dev mailing list