[Mesa-dev] [PATCH 11/20] i965: Add basic-block aware backend_instruction::insert_* methods.

[Matt Turner]

On Wed, Aug 6, 2014 at 6:06 AM, Pohjolainen, Topi
<topi.pohjolainen at intel.com> wrote:
> On Thu, Jul 24, 2014 at 07:54:18PM -0700, Matt Turner wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_shader.cpp | 80 ++++++++++++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_shader.h   |  5 ++
>>  2 files changed, 85 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> index 47535a9..ba93cbc 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> @@ -740,6 +740,86 @@ backend_instruction::has_side_effects() const
>>  }
>>
>>  void
>> +backend_instruction::insert_after(bblock_t *block, backend_instruction *inst)
>> +{
>> +   bool found = false; (void) found;
>> +   foreach_inst_in_block (backend_instruction, i, block) {
>> +      if (this == i) {
>> +         found = true;
>> +      }
>> +   }
>> +   assert(found || !"Instruction not in block");
>> +
>> +   block->end_ip++;
>> +
>> +   for (bblock_t *block_iter = (bblock_t *)block->link.next;
>> +        !block_iter->link.is_tail_sentinel();
>> +        block_iter = (bblock_t *)block_iter->link.next) {
>> +      block_iter->start_ip++;
>> +      block_iter->end_ip++;
>> +   }
>> +
>> +   if (block->end == this)
>> +      block->end = inst;
>> +
>> +   this->insert_after(inst);
>
> If you used "exec_node::insert_after(inst)" instead would you still need the
> using directive in the header?

The using declaration is still needed in other unconverted code. My
plan is to remove it once everything is converted. I'll go ahead and
fix this call to be exec_node::insert_after(inst) (as well as the
other calls to exec_node::insert_before and ::remove in patch 10).

>> +}
>> +
>> +void
>> +backend_instruction::insert_before(bblock_t *block, backend_instruction *inst)
>> +{
>> +   bool found = false; (void) found;
>> +   foreach_inst_in_block (backend_instruction, i, block) {
>> +      if (this == i) {
>> +         found = true;
>> +      }
>> +   }
>> +   assert(found || !"Instruction not in block");
>> +
>> +   block->end_ip++;
>> +
>> +   for (bblock_t *block_iter = (bblock_t *)block->link.next;
>> +        !block_iter->link.is_tail_sentinel();
>> +        block_iter = (bblock_t *)block_iter->link.next) {
>> +      block_iter->start_ip++;
>> +      block_iter->end_ip++;
>> +   }
>> +
>> +   if (block->start == this)
>> +      block->start = inst;
>> +
>> +   this->insert_before(inst);
>> +}
>> +
>> +void
>> +backend_instruction::insert_before(bblock_t *block, exec_list *list)
>> +{
>> +   bool found = false; (void) found;
>> +   foreach_inst_in_block (backend_instruction, i, block) {
>> +      if (this == i) {
>> +         found = true;
>> +      }
>> +   }
>> +   assert(found || !"Instruction not in block");
>
> This is common for all three cases, and could be refactored into its own
> function, say check_inst_in_block(). It would document the seven lines nicely.

Sure, sounds good. Fixed locally with

#ifndef NDEBUG
static bool
inst_is_in_block(const bblock_t *block, const backend_instruction *inst)
{
   bool found = false;
   foreach_inst_in_block (backend_instruction, i, block) {
      if (inst == i) {
         found = true;
      }
   }
   return found;
}
#endif

and then

   assert(inst_is_in_block(block, inst) || !"Instruction not in block");

>> +
>> +   unsigned num_inst = list->length();
>> +
>> +   block->end_ip += num_inst;
>> +
>> +   for (bblock_t *block_iter = (bblock_t *)block->link.next;
>> +        !block_iter->link.is_tail_sentinel();
>> +        block_iter = (bblock_t *)block_iter->link.next) {
>> +      block_iter->start_ip += num_inst;
>> +      block_iter->end_ip += num_inst;
>> +   }
>
> Same here, this iteration is the same and could be its own member with
> arugment telling the adjustment size.

Yes, good idea.

I'll send updated patches for 10 and 11.