On 12 August 2013 13:11, Kenneth Graunke <kenneth at whitecape.org> wrote:
> fs_copy_prop_dataflow::setup_kills() walks through each basic block's
> instructions, looking for instructions which overwrite registers used
> in ACP entries.
>
> This would be fine, except that it didn't exclude the instructions
> which generated the ACP entries in the first place. This meant that
> every copy was killed by its own generating instruction, making the
> dataflow analysis useless.
>
I don't think this is actually a problem, because the bd[b].kills bits are
only used to prevent liveness information from being propagated from
bd[b].livein to bd[b].liveout, and the fs_copy_prop_dataflow constructor
initializes bd[b].liveout with the set of ACP's that are generated from
block b. So the only situation in which your patch would allow liveness
information to be propagated from bd[b].livein to bd[b].liveout is for bits
in bd[b].liveout that are already set.
Or am I misunderstanding things?
>
> To fix this, this patch records the generating instruction in the
> ACP entry. It then skips the kill checks when processing that
> instruction.
>
> Using a void pointer ensures it will only be used for comparisons,
> and not dereferenced.
>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
> src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 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 d8d1546..379605c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
> @@ -42,6 +42,9 @@ namespace { /* avoid conflict with
> opt_copy_propagation_elements */
> struct acp_entry : public exec_node {
> fs_reg dst;
> fs_reg src;
> +
> + /** MOV instruction which generated this copy/ACP entry. */
> + void *inst;
> };
>
> struct block_data {
> @@ -146,8 +149,9 @@ fs_copy_prop_dataflow::setup_kills()
> continue;
>
> for (int i = 0; i < num_acp; i++) {
> - if (inst->overwrites_reg(acp[i]->dst) ||
> - inst->overwrites_reg(acp[i]->src)) {
> + if (inst != acp[i]->inst &&
> + (inst->overwrites_reg(acp[i]->dst) ||
> + inst->overwrites_reg(acp[i]->src))) {
> BITSET_SET(bd[b].kill, i);
> }
> }
> @@ -418,6 +422,7 @@ fs_visitor::opt_copy_propagate_local(void *mem_ctx,
> bblock_t *block,
> acp_entry *entry = ralloc(mem_ctx, acp_entry);
> entry->dst = inst->dst;
> entry->src = inst->src[0];
> + entry->inst = inst;
> acp[entry->dst.reg % ACP_HASH_SIZE].push_tail(entry);
> }
> }
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130815/387e57cd/attachment.html>