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

[Iago Toral]

On Thu, 2016-08-18 at 16:16 -0700, Francisco Jerez wrote:
> 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?

Sure, I am fine with printing it, I was just trying to figure out if
there was a clear case for when that could be useful that I was
missing. I suppose it might help us detect cases where we set wrong
execution masking controls for small instructions in the IR (even in
the case that the hardware doesn't care in the end).

Iago

> > 
> > 
> > > 
> > > > 
> > > > 
> > > > +      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");