[Mesa-dev] [PATCH 04/13] i965/fs_inst: Add an is_copy_payload helper
Jason Ekstrand
jason at jlekstrand.net
Tue May 5 13:13:17 PDT 2015
On Tue, Apr 14, 2015 at 1:28 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, April 01, 2015 06:19:15 PM Jason Ekstrand wrote:
>> This allows us to combine code in CSE and register coalesce
>> ---
>> src/mesa/drivers/dri/i965/brw_fs.cpp | 14 ++++++++++++++
>> src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 18 +-----------------
>> src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp | 14 +-------------
>> src/mesa/drivers/dri/i965/brw_ir_fs.h | 1 +
>> 4 files changed, 17 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 852abbe..4c29cf1 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -517,6 +517,20 @@ fs_inst::is_send_from_grf() const
>> }
>>
>> bool
>> +fs_inst::is_copy_payload() const
>> +{
>> + fs_reg reg = this->src[0];
>> + if (reg.file != GRF || reg.reg_offset != 0 || reg.stride == 0)
>> + return false;
>> +
>> + for (int i = 1; i < this->sources; i++)
>> + if (!this->src[i].equals(::offset(reg, i)))
>> + return false;
>> +
>> + return true;
>> +}
>
> This makes me a bit nervous, because the two pieces of code you're
> combining aren't the same, and the new code is different still:
>
> 1. R.C. enforced that the LOAD_PAYLOAD wrote all elements of the
> destination (via the vgrf size check). CSE did not.
>
> I think both should.
Fixed.
> 2. CSE didn't use equals(), so it allowed things like types to differ,
> or source modifiers...(which are probably bogus on LOAD_PAYLOADs
> anyway, so it may be moot...)
>
> I like using equals(). But...it might make sense to relax BAD_FILE
> checks, i.e. reg_null_d and reg_null_f aren't different for practical
> purposes. (Thinking of header registers...)
It doesn't make sense to have something that copies entirely BAD_FILE.
> 3. CSE relaxed checks on register 0; R.C. checks them all.
>
> I don't think special casing source 0 makes any sense at all.
> Sources 0 .. (inst->header_size - 1) could potentially be BAD_FILE,
> while later sources need to be GRF. So it's not just source 0.
>
> But...it's weird that we're checking inst->src[i] against
> inst->src[0], not inst->dst? I guess it's always done that...
>
> 4. Neither CSE nor R.C. checked inst->src[0].stride at all.
>
> Not really part of a refactoring patch. Also, shouldn't we check this
> on all sources? Note that equals() doesn't check that. Maybe it
> should?
Yeah, I think we should just make equals() check the stride.
> 5. Neither CSE nor R.C. checked inst->src[0].file == GRF.
>
> I supose ensuring src[0].file == GRF ensures src[i].file == GRF too,
> due to the equals() checks...so we don't need to check each of
> them...
RC checks it before is_copy_payload is ever called. I guess CSE
didn't, but I think that was an oversight.
> I'm OK with your approach here of writing a new function that does what
> you want, and replacing both of the old, incomplete ones. But the commit
> message should reflect that.
How about...
This commit adds a new is_copy_payload helper to fs_inst that takes
the place of the similarly named functions in cse and register
coalesce. The two is_copy_payload functions in CSE and register
coalesce were subtly different. The new version unifies the two and
should be more correct.
--Jason
> How about something like:
>
> /**
> * Is this a LOAD_PAYLOAD that performs a self-copy from a VGRF's
> * components to itself?
> */
> bool
> fs_inst::is_copy_payload(unsigned dest_vgrf_size)
> {
> if (opcode != SHADER_OPCODE_LOAD_PAYLOAD || dst.file != GRF)
> return false;
>
> assert(regs_written == sources);
>
> /* Make sure it writes the entire destination register. */
> if (regs_written != dest_vgrf_size)
> return false;
>
> for (int i = 0; i < this->sources; i++) {
> if (src[i].file == BAD_FILE) {
> /* Skip these - XXX: or should we? */
> assert(i < header_size);
> } else if (!src[i].equals(offset(dst, i))) {
> return false;
> }
> }
>
> return true;
> }
>
> I don't know...
>
>> +
>> +bool
>> fs_inst::can_do_source_mods(struct brw_context *brw)
>> {
>> if (brw->gen == 6 && is_math())
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>> index 61837d2..a8aeebf 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_cse.cpp
>> @@ -43,22 +43,6 @@ struct aeb_entry : public exec_node {
>> }
>>
>> static bool
>> -is_copy_payload(const fs_inst *inst)
>> -{
>> - const int reg = inst->src[0].reg;
>> - if (inst->src[0].reg_offset != 0)
>> - return false;
>> -
>> - for (int i = 1; i < inst->sources; i++) {
>> - if (inst->src[i].reg != reg ||
>> - inst->src[i].reg_offset != i) {
>> - return false;
>> - }
>> - }
>> - return true;
>> -}
>> -
>> -static bool
>> is_expression(const fs_inst *const inst)
>> {
>> switch (inst->opcode) {
>> @@ -102,7 +86,7 @@ is_expression(const fs_inst *const inst)
>> case SHADER_OPCODE_COS:
>> return inst->mlen < 2;
>> case SHADER_OPCODE_LOAD_PAYLOAD:
>> - return !is_copy_payload(inst);
>> + return !inst->is_copy_payload();
>> default:
>> return inst->is_send_from_grf() && !inst->has_side_effects();
>> }
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp b/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>> index e3cf2db..884acec 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_register_coalesce.cpp
>> @@ -64,18 +64,6 @@ is_nop_mov(const fs_inst *inst)
>> }
>>
>> static bool
>> -is_copy_payload(const fs_inst *inst)
>> -{
>> - fs_reg reg = inst->src[0];
>> -
>> - for (int i = 0; i < inst->sources; i++)
>> - if (!inst->src[i].equals(offset(reg, i)))
>> - return false;
>> -
>> - return true;
>> -}
>> -
>> -static bool
>> is_coalesce_candidate(const fs_visitor *v, const fs_inst *inst)
>> {
>> if ((inst->opcode != BRW_OPCODE_MOV &&
>> @@ -99,7 +87,7 @@ is_coalesce_candidate(const fs_visitor *v, const fs_inst *inst)
>> if (v->alloc.sizes[inst->src[0].reg] != inst->regs_written)
>> return false;
>>
>> - if (!is_copy_payload(inst)) {
>> + if (!inst->is_copy_payload()) {
>> return false;
>> }
>> }
>> diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> index 9ef1261..c4f5540 100644
>> --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
>> @@ -223,6 +223,7 @@ public:
>> bool overwrites_reg(const fs_reg ®) const;
>> bool is_send_from_grf() const;
>> bool is_partial_write() const;
>> + bool is_copy_payload() const;
>> int regs_read(int arg) const;
>> bool can_do_source_mods(struct brw_context *brw);
>>
>>
More information about the mesa-dev
mailing list