[Mesa-dev] [PATCH] i965/fs: Fail the shader compile instead of asserting when we can't spill

[Francisco Jerez]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Thu, Sep 8, 2016 at 2:39 PM, Francisco Jerez <currojerez at riseup.net>
> wrote:
>
>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>
>> > Blorp doesn't handle spilling so we set allow_spilling to false in that
>> > case.  The blorp 16x MSAA resolve shader spills in 16-wide but not
>> 8-wide.
>> > This commit makes it so that we fail the 16-wide compile and successfully
>> > fall back to 8-wide instead of just assert-failing when trying to compile
>> > the 16-wide shader.
>> >
>> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_fs.cpp | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > index d0b55ae..73aa5d2 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > @@ -5906,6 +5906,11 @@ fs_visitor::allocate_registers(bool
>> allow_spilling)
>> >     }
>> >
>> >     if (!allocated_without_spills) {
>> > +      if (!allow_spilling) {
>> > +         assert(dispatch_width > 8);
>>
>> Not sure it makes sense for the register allocator to set different
>> requirements based on the build dispatch width, wouldn't it make sense
>> to just fail() here consistently if allow_spilling is false regardless
>> of the dispatch width, and let blorp assert fail if none of the dispatch
>> widths it tries out compiles successfully?
>>
>
> Agreed.  I'll drop the assert.
>

Thanks!  With the assert left out patch is:

Reviewed-by: Francisco Jerez <currojerez at riseup.net>

>
>> > +         fail("Failure to register allocate and spilling is not
>> allowed.");
>> > +      }
>> > +
>> >        /* We assume that any spilling is worse than just dropping back to
>> >         * SIMD8.  There's probably actually some intermediate point where
>> >         * SIMD16 with a couple of spills is still better.
>> > @@ -5930,8 +5935,6 @@ fs_visitor::allocate_registers(bool
>> allow_spilling)
>> >        }
>> >     }
>> >
>> > -   assert(last_scratch == 0 || allow_spilling);
>> > -
>> >     /* This must come after all optimization and register allocation,
>> since
>> >      * it inserts dead code that happens to have side effects, and it
>> does
>> >      * so based on the actual physical registers in use.
>> > --
>> > 2.5.0.400.gff86faf
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
-------------- 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/20160908/869041dc/attachment.sig>