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.
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...)
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?
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...
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 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);
>
>
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
