[Mesa-dev] [PATCH 43/95] i965/disasm: print NibCtrl for instructions with execsize 4

[Francisco Jerez]

Iago Toral <itoral at igalia.com> writes:

> On Mon, 2016-08-08 at 15:58 -0700, Francisco Jerez wrote:
>> Iago Toral Quiroga <itoral at igalia.com> writes:
>> 
>> > 
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_disasm.c | 8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c
>> > b/src/mesa/drivers/dri/i965/brw_disasm.c
>> > index c8bdeab..d5e9916 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_disasm.c
>> > +++ b/src/mesa/drivers/dri/i965/brw_disasm.c
>> > @@ -1169,7 +1169,13 @@ qtr_ctrl(FILE *file, const struct
>> > brw_device_info *devinfo, brw_inst *inst)
>> >     int qtr_ctl = brw_inst_qtr_control(devinfo, inst);
>> >     int exec_size = 1 << brw_inst_exec_size(devinfo, inst);
>> >  
>> > -   if (exec_size == 8) {
>> > +   if (exec_size == 4) {
>> I guess it wouldn't hurt to show this for exec_size < 4 too even
>> though
>> it will typically be 1N.
>
> The PRMs say:
>
> "NibCtrl is only allowed for SIMD4 instructions with a DF (Double
> Float) source or destination type"
>
> I don't know if the "only allowed" in that sentence means that it is
> incorrect to set it to to anything else in instructions with an exec
> size < 4, in which case showing it in the disassembly would help find
> bugs although maybe we should just validate for that when we generate
> code, or if it means that the hardware will ignore it completely, in
> which case showing its value does not seem useful for anything.
>
> What do you think?

It's not terribly important, but the main reason I suggested changing
the condition to do the same thing for exec_size < 4 is that it
currently shows no group control information whatsoever for such
instructions.  Regardless of whether the hardware actually does the
right thing or not (the hardware spec is rather ambiguous about it) it
wouldn't hurt to print the information, this is just a disassembler, not
a validator, right?

>
>> > 
>> > +      int nib_ctl = brw_inst_nib_control(devinfo, inst);
>> This may cause an assertion failure on SNB and earlier because the
>> NibCtrl field doesn't exist.  You could do something along the lines
>> of:
>> 
>> > 
>> > const unsigned nib_ctl = devinfo->gen < 7 ? 0 :
>> >                          brw_inst_nib_control(devinfo, inst);
>
> Ah, good to know, thanks!
>
>> > +      if (nib_ctl == 0)
>> > +         string(file, " 1N");
>> > +      else
>> > +         string(file, " 2N");
>> The usual qtr_ctl field is still taken into account by the hardware
>> regardless of whether you use NibCtrl, this should probably be:
>> 
>> > 
>> > format(file, " %dN", qtr_ctl * 2 + nib_ctl + 1);
>
> Yeah, good point.
>
>> > +   } else if (exec_size == 8) {
>> >        switch (qtr_ctl) {
>> >        case 0:
>> >           string(file, " 1Q");
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160818/0d9dd26d/attachment.sig>