[Mesa-dev] [Mesa-stable] [PATCH] i965/fs: Copy the offset when lowering logical pull constant sends

[Jason Ekstrand]

On Wed, Jun 1, 2016 at 3:58 PM, Francisco Jerez <currojerez at riseup.net>
wrote:

> Jason Ekstrand <jason at jlekstrand.net> writes:
>
> > On Wed, Jun 1, 2016 at 3:33 PM, Francisco Jerez <currojerez at riseup.net>
> > wrote:
> >
> >> Jason Ekstrand <jason at jlekstrand.net> writes:
> >>
> >> > This fixes 64 Vulkan CTS tests per gen
> >> >
> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96299
> >> > Cc: "12.0" <mesa-stable at lists.freedesktop.org>
> >> > Cc: Francisco Jerez <currojerez at riseup.net>
> >> > Cc: Mark Janes <mark.a.janes at intel.com>
> >> > ---
> >> >  src/mesa/drivers/dri/i965/brw_fs.cpp | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> > index 00d937e..20bb900 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >> > @@ -4448,6 +4448,14 @@ lower_varying_pull_constant_logical_send(const
> >> fs_builder &bld, fs_inst *inst)
> >> >     const brw_device_info *devinfo = bld.shader->devinfo;
> >> >
> >> >     if (devinfo->gen >= 7) {
> >> > +      /* We are switching the instruction from an ALU-like
> instruction
> >> to a
> >> > +       * send-from-grf instruction.  Since sends can't handle
> strides or
> >> > +       * source modifiers, we have to make a copy of the offset
> source.
> >> > +       */
> >> > +      fs_reg tmp = bld.vgrf(inst->src[1].type);
> >>
> >> I suggest you use a fixed UD type for the temporary (since that is what
> >> the varying pull constant load instruction requires and using any other
> >> type will cause the send-message to silently reinterpret the offset as
> >> something else than what it is.  E.g. think non-32bit integers or
> >> integer vectors, a type-converting move is what you want below in such
> >> cases).  With that change this patch is:
> >>
> >
> > Hrm... How would we ever get anything in there other than a UD?  If
> > something else, say a float, got copy-propagated in here, that seems to
> be
> > a problem with the optimizer not lowering.  I'm fine making the change if
> > you'd like.
> >
> If you don't expect anything else than a UD as source, how about
> 'assert(...type == BRW_REGISTER_TYPE_UD)'?  I think it should be okay
> either way, I suggested using a fixed type above instead because that
> way you'd save an artificial restriction on the source register type of
> this virtual opcode, making the assertion redundant.
>

Fair enough.  Certainly, an implicit format conversion is a better failure
mode than a GPU hang or something similarly stupid.


> > Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> >>
> >
> > Thanks!
> >
> >
> >>
> >> > +      bld.MOV(tmp, inst->src[1]);
> >> > +      inst->src[1] = tmp;
> >> > +
> >> >        inst->opcode = FS_OPCODE_VARYING_PULL_CONSTANT_LOAD_GEN7;
> >> >
> >> >     } else {
> >> > --
> >> > 2.5.0.400.gff86faf
> >> >
> >> > _______________________________________________
> >> > mesa-stable mailing list
> >> > mesa-stable at lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/mesa-stable
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160601/54aecf82/attachment.html>