[Mesa-dev] [PATCH 07/10] i965: Don't copy prop source mods into instructions that can't take them.

[Matt Turner]

On Mon, Sep 23, 2013 at 8:06 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 09/23/2013 04:13 PM, Matt Turner wrote:
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp     |  3 +++
>>  src/mesa/drivers/dri/i965/brw_shader.cpp | 19 +++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_shader.h   |  1 +
>>  src/mesa/drivers/dri/i965/brw_vec4.cpp   |  3 +++
>>  4 files changed, 26 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 6dc37ea..944063c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -366,6 +366,9 @@ fs_visitor::can_do_source_mods(fs_inst *inst)
>>     if (inst->is_send_from_grf())
>>        return false;
>>
>> +   if (!inst->can_do_source_mods())
>> +      return false;
>> +
>>     return true;
>>  }
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> index a558d36..4970607 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> @@ -567,6 +567,25 @@ backend_instruction::is_control_flow()
>>     }
>>  }
>>
>> +bool
>> +backend_instruction::can_do_source_mods()
>> +{
>> +   switch (opcode) {
>> +   case BRW_OPCODE_ADDC:
>> +   case BRW_OPCODE_BFE:
>> +   case BRW_OPCODE_BFI1:
>> +   case BRW_OPCODE_BFI2:
>> +   case BRW_OPCODE_BFREV:
>> +   case BRW_OPCODE_CBIT:
>> +   case BRW_OPCODE_FBH:
>> +   case BRW_OPCODE_FBL:
>> +   case BRW_OPCODE_SUBB:
>> +      return false;
>> +   default:
>> +      return true;
>> +   }
>> +}
>> +
>
> Putting this in backend_instruction makes a ton of sense, but I don't
> like having three methods:
>
> bool backend_instruction::can_do_source_mods()
> bool fs_visitor::can_do_source_mods(fs_inst *)
> bool vec4_visitor::can_do_source_mods(vec4_instruction *)
>
> Since backend_instruction is the obvious place, I'm concerned that
> people will start using that directly...when it doesn't tell the full
> story.  (The other two impose generation-specific restrictions.)

That's probably fair. Maybe if it's called something different?

> Frankly though, I think we should take a different approach: just allow
> source modifiers on all instructions in the backend IRs.  Then, have a
> lowering pass that detects instructions that can't handle source
> modifiers and emits workaround MOVs.  It would run after optimization
> (obviously), but before scheduling or register allocation.
>
> This makes the IR reflect the basic concepts of the assembly without
> needing to capture all of the generation specific details.  Simpler.
>
> The current system of "make every optimization pass try to avoid bad
> things" is also kind of scary.  There are so many opportunities to miss
> something, especially as we add more optimization passes. And if we
> miss a case, we get misrendering or GPU hangs...
>
> We could also handle fix_math_operand in that pass.

I think this raises a lot of questions about what we want our backend
IR to look like.

fix_math_operand also emits a MOV for immediate operands. Should we
move that bit to the proposed pass too? If so, should we allow
immediates sources for three source instructions that don't take
immediates and fix it up at emit time?

fs_visitor/vec4_visitor::can_do_source_mods also return false if
(inst->is_send_from_grf()). What would send r10, -r20 (mlen 2 rlen 4)
mean? Is r20's source modifier just for r20, or also r21, r22, and
r23?

It seems sufficiently simple to encapsulate these restrictions in a
small function, as seen in this patch. Maybe the restrictions will be
more complicated in the future...?

At this point I'm not sure I can see a lot of benefit in a lowering
pass to emit MOVs for instructions that can't take source modifiers.
And we still wouldn't be able to remove all of can_do_source_mods() or
fix_math_operand().