On Mon, Apr 6, 2015 at 6:13 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Mon, Apr 6, 2015 at 4:55 PM, Matt Turner <matts...@gmail.com> wrote:
>> On Fri, Apr 3, 2015 at 2:06 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>>> SEL and MOV instructions, as long as they don't have source modifiers, are
>>> just copying bits around. This commit adds support to copy propagation to
>>> switch the type of a SEL or MOV instruction as needed so that it can
>>> propagate source modifiers. This is needed because NIR generates integer
>>> SEL and MOV instructions whenver it doesn't know what else to generate.
>>>
>>> shader-db results with NIR:
>>> total FS instructions in shared programs: 4360910 -> 4360186 (-0.02%)
>>> FS instructions in affected programs: 59094 -> 58370 (-1.23%)
>>> helped: 341
>>> HURT: 0
>>> GAINED: 2
>>> LOST: 0
>>> ---
>>> .../drivers/dri/i965/brw_fs_copy_propagation.cpp | 33
>>> +++++++++++++++++++---
>>> 1 file changed, 29 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>> b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>> index e8d092c..d321509 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
>>> @@ -275,6 +275,16 @@ is_logic_op(enum opcode opcode)
>>> opcode == BRW_OPCODE_NOT);
>>> }
>>>
>>> +static bool
>>> +can_change_source_types(fs_inst *inst)
>>> +{
>>> + return !inst->src[0].abs && !inst->src[0].negate &&
>>> + (inst->opcode == BRW_OPCODE_MOV ||
>>> + (inst->opcode == BRW_OPCODE_SEL &&
>>> + inst->conditional_mod == BRW_CONDITIONAL_NONE &&
>>
>> I understand what this means, but I'd change this line to
>> inst->predicate (!= BRW_PREDICATE_NONE).
>
> Seems reasonable. I'll do that.
>
>>> + !inst->src[1].abs && !inst->src[1].negate));
>>> +}
>>> +
>>> bool
>>> fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
>>> {
>>> @@ -346,7 +356,9 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg,
>>> acp_entry *entry)
>>> type_sz(inst->src[arg].type)) % type_sz(entry->src.type) != 0)
>>> return false;
>>>
>>> - if (has_source_modifiers && entry->dst.type != inst->src[arg].type)
>>> + if (has_source_modifiers &&
>>> + entry->dst.type != inst->src[arg].type &&
>>> + !can_change_source_types(inst))
>>> return false;
>>>
>>> if (brw->gen >= 8 && (entry->src.negate || entry->src.abs) &&
>>> @@ -412,9 +424,22 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg,
>>> acp_entry *entry)
>>> break;
>>> }
>>>
>>> - if (!inst->src[arg].abs) {
>>> - inst->src[arg].abs = entry->src.abs;
>>> - inst->src[arg].negate ^= entry->src.negate;
>>> + if (has_source_modifiers) {
>>> + if (entry->dst.type != inst->src[arg].type) {
>>> + /* We are propagating source modifiers from a MOV with a different
>>
>> Like Anuj mentioned, MOV or SEL.
>
> You can only propagate *from* a MOV
>
>>> + * type. If we got here, then we can just change the source and
>>> + * destination types of the instruction and keep going.
>>> + */
>>> + assert(can_change_source_types(inst));
>>> + for (int i = 0; i < inst->sources; i++)
>>> + inst->src[i].type = entry->dst.type;
>>
>> Probably put braces araound this.
>
> Sure.
>
>>> + inst->dst.type = entry->dst.type;
>>> + }
>>> +
>>> + if (!inst->src[arg].abs) {
>>> + inst->src[arg].abs = entry->src.abs;
>>> + inst->src[arg].negate ^= entry->src.negate;
>>> + }
>>> }
>>>
>>> return true;
>>> --
>>> 2.3.4
>>
>> Seems totally believable. I'm a little surprised the shader-db results
>> aren't bigger. I'd like to play with it and confirm it handles what I
>> was seeing with SEL instructions.
Ping
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
