[Mesa-dev] [PATCH v3 20/48] intel/fs: Protect opt_algebraic from OOB BROADCAST indices

[Iago Toral]

On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> ---
>  src/intel/compiler/brw_fs.cpp | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index 1c4351b..52079d3 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -2416,8 +2416,14 @@ fs_visitor::opt_algebraic()
>              progress = true;
>           } else if (inst->src[1].file == IMM) {
>              inst->opcode = BRW_OPCODE_MOV;
> -            inst->src[0] = component(inst->src[0],
> -                                     inst->src[1].ud);
> +            /* It's possible that the selected component will be too
> large and
> +             * overflow the register.  If this happens and we some
> how manage
> +             * to constant fold it in and get here, it would cause
> an assert
> +             * in component() below.  Instead, just let it wrap
> around if it
> +             * goes over exec_size.
> +             */

component() is really a horiz_offset() call which is in turn a
byte_offset() call, which doesn't assert on anything other than invalid
register files. I guess you mean that the byte offset computed by the
component() call below can later lead to hitting assertions as we
attempt to read outside the allocated space for the vgrf, right?

My question is whether this is supposed to happen at all, it seems like
we should not be emitting broadcast operations like this at all since
they are invalid and here we are instead papering over that bug.

> +            const unsigned comp = inst->src[1].ud & (inst->exec_size 
> - 1);
> +            inst->src[0] = component(inst->src[0], comp);
>              inst->sources = 1;
>              inst->force_writemask_all = true;
>              progress = true;