[Mesa-dev] [PATCH 2/4] i965: Fix multiplication of immediates on Cherryview/Broxton.

[Kenneth Graunke]

On Saturday, June 11, 2016 4:06:32 PM PDT Jordan Justen wrote:
> On 2016-06-10 14:19:44, Kenneth Graunke wrote:
> > Cherryview and Broxton don't support DW x DW multiplication.  We have
> > piles of code to handle this, but apparently weren't retyping in the
> > immediate case.
> > 
> > For example,
> > tests/spec/arb_tessellation_shader/execution/dvec3-vs-tcs-tes
> > makes the simulator angry about instructions such as:
> > 
> >    mul(8) r18<1>:D r10.0<8;8,1>:D 0x00000003:D
> > 
> > Just retype to UW.  It should be safe everywhere.
> > 
> > Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95462
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs.cpp | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > index 4b29ee5..13246c2 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> > @@ -3564,7 +3564,10 @@ fs_visitor::lower_integer_multiplication()
> >                 ibld.MOV(imm, inst->src[1]);
> >                 ibld.MUL(inst->dst, imm, inst->src[0]);
> >              } else {
> > -               ibld.MUL(inst->dst, inst->src[0], inst->src[1]);
> > +               const bool ud = (inst->src[1].type == BRW_REGISTER_TYPE_UD);
> > +               ibld.MUL(inst->dst, inst->src[0],
> > +                        ud ? brw_imm_uw(inst->src[1].ud)
> > +                           : brw_imm_w(inst->src[1].d));
> 
> This change looks fine, but will it actually be possible to hit this
> code path for negative numbers? Above, we have:
> 
>    if (inst->src[1].file == IMM &&
>        inst->src[1].ud < (1 << 16)) {
> 
> Bit 31 would be set if inst->src[1].d has a negative number.
> 
> -Jordan

Good catch.  I don't think it can be hit for negative numbers currently.
(Perhaps we should improve that check so we can handle those, someday?)

But we can still get positive numbers with D type that fit in 16 bits.
Here, I was just trying to preserve the signedness of the original type
so that we convert a D * D multiply into D * W rather than D * UW.  It
seemed safer.  Maybe it doesn't matter, though.

I've updated the last line of the commit message to:

    Just retype to W or UW.  It should be safe on all platforms.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160617/9da18c18/attachment.sig>