[Mesa-dev] [PATCH 21/30] i965/fs: Add live interval validation pass.

[Matt Turner]

On Sun, Mar 13, 2016 at 8:47 PM, Francisco Jerez <currojerez at riseup.net> wrote:
> This could be improved somewhat with additional validation of the
> calculated live in/out sets and by checking that the calculated live
> intervals are minimal (which isn't strictly necessary to guarantee the
> correctness of the program).  This should be good enough though to
> catch accidental use of stale liveness results due to missing or
> incorrect analysis invalidation.
> ---
>  .../drivers/dri/i965/brw_fs_live_variables.cpp     | 41 ++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_fs_live_variables.h  |  3 ++
>  2 files changed, 44 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp b/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp
> index 4b0943f..215349a 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_live_variables.cpp
> @@ -304,6 +304,47 @@ fs_live_variables::~fs_live_variables()
>     ralloc_free(mem_ctx);
>  }
>
> +static bool
> +check_register_live_range(const fs_live_variables *live, int ip,
> +                          const fs_reg &reg, unsigned n)
> +{
> +   const unsigned var = live->var_from_reg(reg);
> +
> +   if (var + n > unsigned(live->num_vars) ||
> +       live->vgrf_start[reg.nr] > ip || live->vgrf_end[reg.nr] < ip)
> +      return false;
> +
> +   for (unsigned j = 0; j < n; j++) {
> +      if (live->start[var + j] > ip || live->end[var + j] < ip)
> +         return false;
> +   }
> +
> +   return true;
> +}
> +
> +bool
> +fs_live_variables::validate(const backend_shader *s) const
> +{
> +   int ip = 0;
> +
> +   foreach_block_and_inst(block, fs_inst, inst, s->cfg) {
> +      for (unsigned i = 0; i < inst->sources; i++) {
> +         if (inst->src[i].file == VGRF &&
> +             !check_register_live_range(this, ip,
> +                                        inst->src[i], inst->regs_read(i)))
> +            return false;
> +      }
> +
> +      if (inst->dst.file == VGRF &&
> +         !check_register_live_range(this, ip, inst->dst, inst->regs_written))

Looks like the indentation is slightly off on this line.

> +         return false;
> +
> +      ip++;
> +   }
> +
> +   return true;
> +}
> +
>  void
>  fs_visitor::invalidate_live_intervals()
>  {
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_live_variables.h b/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
> index e1cd12c..c2a3c63 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
> @@ -69,6 +69,9 @@ public:
>     fs_live_variables(const backend_shader *s);
>     ~fs_live_variables();
>
> +   bool
> +   validate(const backend_shader *s) const;

Return type on its own line -- intentional? Others, seen below, are on
the same line. I'd have put it on the same line.

> +
>     bool vars_interfere(int a, int b) const;
>     bool vgrfs_interfere(int a, int b) const;
>     int var_from_reg(const fs_reg &reg) const
> --