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

Submitted by Iago Toral Quiroga on July 19, 2016, 10:40 a.m.

Details

Message ID 1468924892-6910-44-git-send-email-itoral@igalia.com
State New
Headers show
Series "i965 Haswell ARB_gpu_shader_fp64 / OpenGL 4.0" ( rev: 2 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga July 19, 2016, 10:40 a.m.
---
 src/mesa/drivers/dri/i965/brw_disasm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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) {
+      int nib_ctl = brw_inst_nib_control(devinfo, inst);
+      if (nib_ctl == 0)
+         string(file, " 1N");
+      else
+         string(file, " 2N");
+   } else if (exec_size == 8) {
       switch (qtr_ctl) {
       case 0:
          string(file, " 1Q");

Comments

Iago Toral Quiroga <itoral@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.

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

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

> +   } else if (exec_size == 8) {
>        switch (qtr_ctl) {
>        case 0:
>           string(file, " 1Q");
> -- 
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Mon, 2016-08-08 at 15:58 -0700, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral@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?

> > 
> > +      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");
Iago Toral <itoral@igalia.com> writes:

> On Mon, 2016-08-08 at 15:58 -0700, Francisco Jerez wrote:
>> Iago Toral Quiroga <itoral@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");
On Thu, 2016-08-18 at 16:16 -0700, Francisco Jerez wrote:
> Iago Toral <itoral@igalia.com> writes:
> 
> > 
> > On Mon, 2016-08-08 at 15:58 -0700, Francisco Jerez wrote:
> > > 
> > > Iago Toral Quiroga <itoral@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");