Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes: > From: Iago Toral Quiroga <ito...@igalia.com> > > We were not invalidating entries with a src that reads more than one register > when we find writes that overwrite any register read by entry->src after > the first. This leads to incorrect copy propagation because we re-use > entries from the ACP that have been partially invalidated. Same thing for > entries with a dst that writes to more than one register. > --- > .../drivers/dri/i965/brw_fs_copy_propagation.cpp | 35 > ++++++++++++++++------ > 1 file changed, 26 insertions(+), 9 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 23df877..fe37676 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp > @@ -44,6 +44,7 @@ struct acp_entry : public exec_node { > fs_reg dst; > fs_reg src; > uint8_t regs_written; > + uint8_t regs_read; > enum opcode opcode; > bool saturate; > }; > @@ -768,18 +769,32 @@ fs_visitor::opt_copy_propagate_local(void > *copy_prop_ctx, bblock_t *block, > /* kill the destination from the ACP */ > if (inst->dst.file == VGRF) { > foreach_in_list_safe(acp_entry, entry, &acp[inst->dst.nr % > ACP_HASH_SIZE]) { > - if (inst->overwrites_reg(entry->dst)) { > - entry->remove(); > - } > - } > + fs_reg tmp = entry->dst; > + for (int n = 0; n < entry->regs_written; n++) { > + if (inst->overwrites_reg(tmp)) { > + entry->remove(); > + break; > + } > + tmp.reg_offset++; > + } > + }
The loop shouldn't be necessary, I suggest we do something like: | if (regions_overlap(entry->dst, entry->regs_written, | inst->dst, inst->regs_written) { | entry->remove(); | break; | } where: | inline bool | regions_overlap(const fs_reg &r, unsigned n, const fs_reg &s, unsigned m) | { | return r.file == s.file && r.nr == s.nr && | !(r.reg_offset + n < s.reg_offset || | s.reg_offset + m < r.reg_offset); | } Alternatively you could extend 'fs_inst::overwrites_reg()' to take a register range instead of a single register -- Whatever you do it would be nice to see a v2 of the patch before you push. > > /* Oops, we only have the chaining hash based on the destination, > not > * the source, so walk across the entire table. > */ > for (int i = 0; i < ACP_HASH_SIZE; i++) { > foreach_in_list_safe(acp_entry, entry, &acp[i]) { > - if (inst->overwrites_reg(entry->src)) > - entry->remove(); > + /* Make sure we kill the entry if this instructions overwrites > + * _any_ of the registers that it reads > + */ > + fs_reg tmp = entry->src; > + for (int n = 0; n < entry->regs_read; n++) { > + if (inst->overwrites_reg(tmp)) { > + entry->remove(); > + break; > + } > + tmp.reg_offset++; > + } Use 'regions_overlap' here too instead of a loop. > } > } > } > @@ -788,10 +803,11 @@ fs_visitor::opt_copy_propagate_local(void > *copy_prop_ctx, bblock_t *block, > * operand of another instruction, add it to the ACP. > */ > if (can_propagate_from(inst)) { > - acp_entry *entry = ralloc(copy_prop_ctx, acp_entry); > - entry->dst = inst->dst; > - entry->src = inst->src[0]; > + acp_entry *entry = ralloc(copy_prop_ctx, acp_entry); > + entry->dst = inst->dst; > + entry->src = inst->src[0]; > entry->regs_written = inst->regs_written; > + entry->regs_read = inst->regs_read(0); > entry->opcode = inst->opcode; > entry->saturate = inst->saturate; > acp[entry->dst.nr % ACP_HASH_SIZE].push_tail(entry); > @@ -807,6 +823,7 @@ fs_visitor::opt_copy_propagate_local(void *copy_prop_ctx, > bblock_t *block, > entry->dst.reg_offset = offset; > entry->src = inst->src[i]; > entry->regs_written = regs_written; > + entry->regs_read = inst->regs_read(i); > entry->opcode = inst->opcode; > if (!entry->dst.equals(inst->src[i])) { > acp[entry->dst.nr % ACP_HASH_SIZE].push_tail(entry); > -- > 2.5.0 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev