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

[Francisco Jerez]

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.

> 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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160601/a53e9736/attachment.sig>