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

[Connor Abbott]

On Thu, Dec 18, 2014 at 1:49 PM, Eric Anholt <eric at anholt.net> wrote:
> 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.

No, this isn't validating what you think it's validating. This
function is called every time a variable dereference happens, and
checks that the dereference is in the same function implementation as
the definition of the variable. Maybe we can add a && "deferencing a
local variable defined in a different function" to the assert to make
it clear what what's happening.

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

That check happens at the beginning of validate_reg_dest() and then we
add the destination to reg_state->defs a few lines down. A similar
thing happens for the uses.