[Mesa-dev,41/95] i965/vec4: make the generator set correct NibCtrl for SIMD4 DF instructions

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

Details

Message ID 1468924892-6910-42-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.
From the HSW PRM, Command Reference, QtrCtrl:

   "NibCtrl is only allowed for SIMD4 instructions with a DF (Double Float)
    source or destination type."

v2 (Samuel): Assert that the type is DF.

Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
---
 src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 9 +++++++++
 1 file changed, 9 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
index c6e040e..dc4f6db 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
@@ -1494,6 +1494,7 @@  generate_code(struct brw_codegen *p,
       brw_set_default_saturate(p, inst->saturate);
       brw_set_default_mask_control(p, inst->force_writemask_all);
       brw_set_default_acc_write_control(p, inst->writes_accumulator);
+      brw_set_default_group(p, 0);
 
       assert(inst->base_mrf + inst->mlen <= BRW_MAX_MRF(devinfo->gen));
       assert(inst->mlen <= BRW_MAX_MSG_LENGTH);
@@ -1529,6 +1530,14 @@  generate_code(struct brw_codegen *p,
       } else {
          assert(inst->exec_size == 8 || inst->exec_size == 4);
          brw_set_default_exec_size(p, cvt(inst->exec_size) - 1);
+         if (inst->exec_size == 4 && !inst->force_writemask_all) {
+            assert(inst->dst.type == BRW_REGISTER_TYPE_DF ||
+                   inst->src[0].type == BRW_REGISTER_TYPE_DF ||
+                   inst->src[1].type == BRW_REGISTER_TYPE_DF ||
+                   inst->src[2].type == BRW_REGISTER_TYPE_DF);
+            assert(inst->group == 0 || inst->group == 4);
+            brw_inst_set_group(p->devinfo, p->current, inst->group);
+         }
       }
 
       switch (inst->opcode) {

Comments

Iago Toral Quiroga <itoral@igalia.com> writes:

> From the HSW PRM, Command Reference, QtrCtrl:
>
>    "NibCtrl is only allowed for SIMD4 instructions with a DF (Double Float)
>     source or destination type."
>
> v2 (Samuel): Assert that the type is DF.
>
> Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> ---
>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> index c6e040e..dc4f6db 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> @@ -1494,6 +1494,7 @@ generate_code(struct brw_codegen *p,
>        brw_set_default_saturate(p, inst->saturate);
>        brw_set_default_mask_control(p, inst->force_writemask_all);
>        brw_set_default_acc_write_control(p, inst->writes_accumulator);
> +      brw_set_default_group(p, 0);
>
>        assert(inst->base_mrf + inst->mlen <= BRW_MAX_MRF(devinfo->gen));
>        assert(inst->mlen <= BRW_MAX_MSG_LENGTH);
> @@ -1529,6 +1530,14 @@ generate_code(struct brw_codegen *p,
>        } else {
>           assert(inst->exec_size == 8 || inst->exec_size == 4);
>           brw_set_default_exec_size(p, cvt(inst->exec_size) - 1);
> +         if (inst->exec_size == 4 && !inst->force_writemask_all) {
> +            assert(inst->dst.type == BRW_REGISTER_TYPE_DF ||
> +                   inst->src[0].type == BRW_REGISTER_TYPE_DF ||
> +                   inst->src[1].type == BRW_REGISTER_TYPE_DF ||
> +                   inst->src[2].type == BRW_REGISTER_TYPE_DF);
> +            assert(inst->group == 0 || inst->group == 4);
> +            brw_inst_set_group(p->devinfo, p->current, inst->group);

I think this should be unconditional (e.g. if somebody tries to set
group == 4 on an instruction with exec_size == 8 it would be nice to get
an assertion failure instead of the group value being silently
ignored).  How about you replace the 'brw_set_default_group(p, 0)' line
above with:

|
|       assert(inst->group % inst->exec_size == 0);
|       assert(inst->group % 8 == 0 ||
|              inst->dst.type == BRW_REGISTER_TYPE_DF ||
|              inst->src[0].type == BRW_REGISTER_TYPE_DF ||
|              inst->src[1].type == BRW_REGISTER_TYPE_DF ||
|              inst->src[2].type == BRW_REGISTER_TYPE_DF);
|       brw_set_default_group(p, inst->group);

and then drop this hunk?

>        }
>  
>        switch (inst->opcode) {
> -- 
> 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:26 -0700, Francisco Jerez wrote:
> Iago Toral Quiroga <itoral@igalia.com> writes:
> 
> > 
> > From the HSW PRM, Command Reference, QtrCtrl:
> > 
> >    "NibCtrl is only allowed for SIMD4 instructions with a DF
> > (Double Float)
> >     source or destination type."
> > 
> > v2 (Samuel): Assert that the type is DF.
> > 
> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
> > ---
> >  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > index c6e040e..dc4f6db 100644
> > --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
> > @@ -1494,6 +1494,7 @@ generate_code(struct brw_codegen *p,
> >        brw_set_default_saturate(p, inst->saturate);
> >        brw_set_default_mask_control(p, inst->force_writemask_all);
> >        brw_set_default_acc_write_control(p, inst-
> > >writes_accumulator);
> > +      brw_set_default_group(p, 0);
> > 
> >        assert(inst->base_mrf + inst->mlen <= BRW_MAX_MRF(devinfo-
> > >gen));
> >        assert(inst->mlen <= BRW_MAX_MSG_LENGTH);
> > @@ -1529,6 +1530,14 @@ generate_code(struct brw_codegen *p,
> >        } else {
> >           assert(inst->exec_size == 8 || inst->exec_size == 4);
> >           brw_set_default_exec_size(p, cvt(inst->exec_size) - 1);
> > +         if (inst->exec_size == 4 && !inst->force_writemask_all) {
> > +            assert(inst->dst.type == BRW_REGISTER_TYPE_DF ||
> > +                   inst->src[0].type == BRW_REGISTER_TYPE_DF ||
> > +                   inst->src[1].type == BRW_REGISTER_TYPE_DF ||
> > +                   inst->src[2].type == BRW_REGISTER_TYPE_DF);
> > +            assert(inst->group == 0 || inst->group == 4);
> > +            brw_inst_set_group(p->devinfo, p->current, inst-
> > >group);
> I think this should be unconditional (e.g. if somebody tries to set
> group == 4 on an instruction with exec_size == 8 it would be nice to
> get
> an assertion failure instead of the group value being silently
> ignored).  How about you replace the 'brw_set_default_group(p, 0)'
> line
> above with:
> 
> > 
> > 
> >       assert(inst->group % inst->exec_size == 0);
> >       assert(inst->group % 8 == 0 ||
> >              inst->dst.type == BRW_REGISTER_TYPE_DF ||
> >              inst->src[0].type == BRW_REGISTER_TYPE_DF ||
> >              inst->src[1].type == BRW_REGISTER_TYPE_DF ||
> >              inst->src[2].type == BRW_REGISTER_TYPE_DF);
> >       brw_set_default_group(p, inst->group);
> and then drop this hunk?

Yes, that looks like better. Thanks!

I suppose that setting the group won't have any effect  if
force_writemask_all is set as too so there is no need to void setting
it in that case, right?

> > 
> >        }
> >  
> >        switch (inst->opcode) {
Iago Toral <itoral@igalia.com> writes:

> On Mon, 2016-08-08 at 15:26 -0700, Francisco Jerez wrote:
>> Iago Toral Quiroga <itoral@igalia.com> writes:
>> 
>> > 
>> > From the HSW PRM, Command Reference, QtrCtrl:
>> > 
>> >    "NibCtrl is only allowed for SIMD4 instructions with a DF
>> > (Double Float)
>> >     source or destination type."
>> > 
>> > v2 (Samuel): Assert that the type is DF.
>> > 
>> > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias@igalia.com>
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> > 
>> > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
>> > b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
>> > index c6e040e..dc4f6db 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp
>> > @@ -1494,6 +1494,7 @@ generate_code(struct brw_codegen *p,
>> >        brw_set_default_saturate(p, inst->saturate);
>> >        brw_set_default_mask_control(p, inst->force_writemask_all);
>> >        brw_set_default_acc_write_control(p, inst-
>> > >writes_accumulator);
>> > +      brw_set_default_group(p, 0);
>> > 
>> >        assert(inst->base_mrf + inst->mlen <= BRW_MAX_MRF(devinfo-
>> > >gen));
>> >        assert(inst->mlen <= BRW_MAX_MSG_LENGTH);
>> > @@ -1529,6 +1530,14 @@ generate_code(struct brw_codegen *p,
>> >        } else {
>> >           assert(inst->exec_size == 8 || inst->exec_size == 4);
>> >           brw_set_default_exec_size(p, cvt(inst->exec_size) - 1);
>> > +         if (inst->exec_size == 4 && !inst->force_writemask_all) {
>> > +            assert(inst->dst.type == BRW_REGISTER_TYPE_DF ||
>> > +                   inst->src[0].type == BRW_REGISTER_TYPE_DF ||
>> > +                   inst->src[1].type == BRW_REGISTER_TYPE_DF ||
>> > +                   inst->src[2].type == BRW_REGISTER_TYPE_DF);
>> > +            assert(inst->group == 0 || inst->group == 4);
>> > +            brw_inst_set_group(p->devinfo, p->current, inst-
>> > >group);
>> I think this should be unconditional (e.g. if somebody tries to set
>> group == 4 on an instruction with exec_size == 8 it would be nice to
>> get
>> an assertion failure instead of the group value being silently
>> ignored).  How about you replace the 'brw_set_default_group(p, 0)'
>> line
>> above with:
>> 
>> > 
>> > 
>> >       assert(inst->group % inst->exec_size == 0);
>> >       assert(inst->group % 8 == 0 ||
>> >              inst->dst.type == BRW_REGISTER_TYPE_DF ||
>> >              inst->src[0].type == BRW_REGISTER_TYPE_DF ||
>> >              inst->src[1].type == BRW_REGISTER_TYPE_DF ||
>> >              inst->src[2].type == BRW_REGISTER_TYPE_DF);
>> >       brw_set_default_group(p, inst->group);
>> and then drop this hunk?
>
> Yes, that looks like better. Thanks!
>
> I suppose that setting the group won't have any effect  if
> force_writemask_all is set as too so there is no need to void setting
> it in that case, right?
>
Yeah, it shouldn't make much of a difference, I wouldn't bother not to
set it.

>> > 
>> >        }
>> >  
>> >        switch (inst->opcode) {