[Mesa-dev] [PATCH 2/2] tgsi: Prevent emission of instructions with empty writemask.
Roland Scheidegger
sroland at vmware.com
Fri Nov 22 08:06:03 PST 2013
On 11/22/2013 03:03 PM, Jose Fonseca wrote:
> Thanks for reviewing.
>
> ----- Original Message -----
>> On 11/21/2013 02:01 PM, jfonseca at vmware.com wrote:
>>> From: José Fonseca <jfonseca at vmware.com>
>>>
>>> These degenerate instructions can often be emitted by state trackers
>>> when the semantics of instructions don't match precisely.
>>> ---
>>> src/gallium/auxiliary/tgsi/tgsi_ureg.c | 8 ++++++++
>>> src/gallium/auxiliary/tgsi/tgsi_ureg.h | 22 ++++++++++++++++++++++
>>> 2 files changed, 30 insertions(+)
>>>
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> index 432ed00..f06858e 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_ureg.c
>>> @@ -1113,6 +1113,10 @@ ureg_insn(struct ureg_program *ureg,
>>> boolean negate = FALSE;
>>> unsigned swizzle[4] = { 0 };
>>>
>>> + if (nr_dst && ureg_dst_is_empty(dst[0])) {
>>> + return;
>>> + }
>>>
>> This is technically not really correct, as the instruction may have
>> multiple destinations.
>> Not saying we really have any such instructions (or even that we should
>> have them), but this helper (and the one below) has code to deal with
>> multiple destinations, so that's a bit inconsistent.
>
> The assumption there is only one dst is already there. See the code immediately below
Oh right! Makes me wonder if we shouldn't get rid of multi-dst business
completely.
>
>
> saturate = nr_dst ? dst[0].Saturate : FALSE;
> predicate = nr_dst ? dst[0].Predicate : FALSE;
>
> I rather commit this as is now, as the multiple destination is not a real problem ATM, while the empty writemask do seem to arise due to me earlier commit.
>
>> Also, there are instructions which have just one dst reg but side
>> effects, though it may be restricted to OPCODE_POPA (and I wouldn't mind
>> seeing it disappear) (I think the fence/atomic instructions might be
>> fine and at first glance I don't see anything else).
>
> POPA is not used anywhere and can be removed. I'll do that in another commit.
Ok thanks.
Roland
>
> Jose
>
>>
>>> saturate = nr_dst ? dst[0].Saturate : FALSE;
>>> predicate = nr_dst ? dst[0].Predicate : FALSE;
>>> if (predicate) {
>>> @@ -1162,6 +1166,10 @@ ureg_tex_insn(struct ureg_program *ureg,
>>> boolean negate = FALSE;
>>> unsigned swizzle[4] = { 0 };
>>>
>>> + if (nr_dst && ureg_dst_is_empty(dst[0])) {
>>> + return;
>>> + }
>> Same as above.
>>
>>> saturate = nr_dst ? dst[0].Saturate : FALSE;
>>> predicate = nr_dst ? dst[0].Predicate : FALSE;
>>> if (predicate) {
>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_ureg.h
>>> b/src/gallium/auxiliary/tgsi/tgsi_ureg.h
>>> index cf0c75e..d973edb 100644
>>> --- a/src/gallium/auxiliary/tgsi/tgsi_ureg.h
>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_ureg.h
>>> @@ -454,6 +454,16 @@ ureg_imm1i( struct ureg_program *ureg,
>>> return ureg_DECL_immediate_int( ureg, &a, 1 );
>>> }
>>>
>>> +/* Where the destination register has a valid file, but an empty
>>> + * writemask.
>>> + */
>>> +static INLINE boolean
>>> +ureg_dst_is_empty( struct ureg_dst dst )
>>> +{
>>> + return dst.File != TGSI_FILE_NULL &&
>>> + dst.WriteMask == 0;
>>> +}
>>> +
>>> /***********************************************************************
>>> * Functions for patching up labels
>>> */
>>> @@ -650,6 +660,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -673,6 +684,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -697,6 +709,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -723,6 +736,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -750,6 +764,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> unsigned target = TGSI_TEXTURE_UNKNOWN; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -777,6 +792,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -805,6 +821,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> unsigned target = TGSI_TEXTURE_UNKNOWN; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -835,6 +852,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -866,6 +884,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> unsigned target = TGSI_TEXTURE_UNKNOWN; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -897,6 +916,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -928,6 +948,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> { \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>> @@ -960,6 +981,7 @@ static INLINE void ureg_##op( struct ureg_program
>>> *ureg, \
>>> unsigned opcode = TGSI_OPCODE_##op; \
>>> unsigned target = TGSI_TEXTURE_UNKNOWN; \
>>> struct ureg_emit_insn_result insn; \
>>> + if (ureg_dst_is_empty(dst)) return; \
>>> insn = ureg_emit_insn(ureg, \
>>> opcode, \
>>> dst.Saturate, \
>>>
>>
>> Otherwise this looks good to me, though I wouldn't insist it's really
>> necessary to prune such instructions.
>>
>> Roland
>>
More information about the mesa-dev
mailing list