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

[Francisco Jerez]

Matt Turner <mattst88 at gmail.com> writes:

> 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.

Nope, it wasn't intentional, just got confused by having to put the
return type on a separate line but only half of the time.  Fixed
locally.

>
>> +
>>     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
>> --
-------------- 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/20160314/7797e2c3/attachment.sig>