[Mesa-dev] [PATCH 007/133] nir: add a validation pass

[Eric Anholt]

Connor Abbott <cwabbott0 at gmail.com> writes:

> On Thu, Dec 18, 2014 at 2:01 AM, Eric Anholt <eric at anholt.net> wrote:
>> Jason Ekstrand <jason at jlekstrand.net> writes:
>>
>>> From: Connor Abbott <connor.abbott at intel.com>
>>>
>>> This is similar to ir_validate.cpp.
>>>
>>> v2: Jason Ekstrand <jason.ekstrand at intel.com>:
>>>    whitespace fixes
>>
>> I have again not reviewed the control flow bits.  Couple of questions I
>> had, though:
>>
>>> +static void
>>> +validate_var_use(nir_variable *var, validate_state *state)
>>> +{
>>> +   if (var->data.mode == nir_var_local) {
>>> +      struct hash_entry *entry =
>>> +         _mesa_hash_table_search(state->var_defs, _mesa_hash_pointer(var),
>>> +                                 var);
>>> +
>>> +      assert(entry);
>>> +      assert((nir_function_impl *) entry->data == state->impl);
>>> +   }
>>> +}
>>
>> Is there guaranteed to be a def of a local variable before a use?  It
>> would be undefined execution behavior, but not assertion failure
>> quality, right?
>
> Yes, that's correct - there are no guarantees about this for variables
> and registers. For SSA values, the definition should always dominate
> the use (see the TODO about that) because a lot of SSA algorithms
> assume that, so we model the use-before-def case by pointing the use
> to a nir_ssa_undef_instr.

OK, so it seems like this validation needs to be dropped.

>>> +static void
>>> +postvalidate_reg_decl(nir_register *reg, validate_state *state)
>>> +{
>>> +   struct hash_entry *entry = _mesa_hash_table_search(state->regs,
>>> +                                                      _mesa_hash_pointer(reg),
>>> +                                                      reg);
>>> +
>>> +   reg_validate_state *reg_state = (reg_validate_state *) entry->data;
>>> +
>>> +   if (reg_state->uses->entries != reg->uses->entries) {
>>> +      printf("extra entries in register uses:\n");
>>> +      struct set_entry *entry;
>>> +      set_foreach(reg->uses, entry) {
>>> +         struct set_entry *entry2 =
>>> +            _mesa_set_search(reg_state->uses, _mesa_hash_pointer(entry->key),
>>> +                             entry->key);
>>> +
>>> +         if (entry2 == NULL) {
>>> +            printf("%p\n", entry->key);
>>> +         }
>>> +      }
>>> +
>>> +      abort();
>>> +   }
>>> +
>>> +   if (reg_state->defs->entries != reg->defs->entries) {
>>> +      printf("extra entries in register defs:\n");
>>> +      struct set_entry *entry;
>>> +      set_foreach(reg->defs, entry) {
>>> +         struct set_entry *entry2 =
>>> +            _mesa_set_search(reg_state->defs, _mesa_hash_pointer(entry->key),
>>> +                             entry->key);
>>> +
>>> +         if (entry2 == NULL) {
>>> +            printf("%p\n", entry->key);
>>> +         }
>>> +      }
>>
>> Couldn't these failures go the other way and there be, for example,
>> defs that weren't tracked in the reg?
>>
>> (Not necessarily important to fix, since you'll at least get the
>> abort())
>
> Yeah, the point here is that we've already validated that all the
> actual definitions (i.e. everything in reg_state->defs) are already in
> reg->defs, so once we've gotten to this point the only possible reason
> for them not being the same is that reg->defs has extra entries, which
> we check for here.

I must have skimmed right past that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141218/2a799a9a/attachment.sig>