Module: Mesa Branch: staging/22.2 Commit: 1959ee01837283498379ad21d7b45aec87d2b2d2 URL: http://cgit.freedesktop.org/mesa/mesa/commit/?id=1959ee01837283498379ad21d7b45aec87d2b2d2
Author: Alyssa Rosenzweig <[email protected]> Date: Thu Jul 21 16:48:36 2022 -0400 pan/bi: Consider all dests in helper_block_update If an instruction has multiple destinations and *any* of them are needed by helper invocations, we should keep helper invocations alive. This is a bug fix. Consider the GLSL: first = texture(sampler, ...); float res = texture(sampler, vec2(first.y)).x + first.x; Corresponding to the IR: first = ... x, y, z, w = SPLIT first second = TEX y, y x', y', z', w' = SPLIT second FADD res, x, x' Here, x is not required by helper invocations (the coordinates to TEX) while y is required. If we only look at only the first destinations, we incorrectly decide that first is not required and fail to set the .skip bit, leading to incorrect results. Fixes: 5febeae58e0 ("pan/bi: Emit collect and split") Signed-off-by: Alyssa Rosenzweig <[email protected]> Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/17794> (cherry picked from commit d0aaf52602341705a915d85fe295341de0744bd2) --- .pick_status.json | 2 +- src/panfrost/bifrost/bi_helper_invocations.c | 41 +++++++++++++++++----------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/.pick_status.json b/.pick_status.json index 9c437d03814..bab2e0e3772 100644 --- a/.pick_status.json +++ b/.pick_status.json @@ -5539,7 +5539,7 @@ "description": "pan/bi: Consider all dests in helper_block_update", "nominated": true, "nomination_type": 1, - "resolution": 0, + "resolution": 1, "main_sha": null, "because_sha": "5febeae58e0a133f048cb0e2d1ef45549851bea4" }, diff --git a/src/panfrost/bifrost/bi_helper_invocations.c b/src/panfrost/bifrost/bi_helper_invocations.c index 038f48b11f0..82eda81a4d9 100644 --- a/src/panfrost/bifrost/bi_helper_invocations.c +++ b/src/panfrost/bifrost/bi_helper_invocations.c @@ -198,20 +198,24 @@ bi_helper_block_update(BITSET_WORD *deps, bi_block *block) bool progress = false; bi_foreach_instr_in_block_rev(block, I) { - /* If our destination is required by helper invocation... */ - if (I->dest[0].type != BI_INDEX_NORMAL) - continue; - - if (!BITSET_TEST(deps, bi_get_node(I->dest[0]))) - continue; - - /* ...so are our sources */ - bi_foreach_src(I, s) { - if (I->src[s].type == BI_INDEX_NORMAL) { - unsigned node = bi_get_node(I->src[s]); - progress |= !BITSET_TEST(deps, node); - BITSET_SET(deps, node); + /* If a destination is required by helper invocation... */ + bi_foreach_dest(I, d) { + if (bi_is_null(I->dest[d])) + continue; + + if (!BITSET_TEST(deps, bi_get_node(I->dest[d]))) + continue; + + /* ...so are the sources */ + bi_foreach_src(I, s) { + if (I->src[s].type == BI_INDEX_NORMAL) { + unsigned node = bi_get_node(I->src[s]); + progress |= !BITSET_TEST(deps, node); + BITSET_SET(deps, node); + } } + + break; } } @@ -228,7 +232,6 @@ bi_analyze_helper_requirements(bi_context *ctx) * derivatives */ bi_foreach_instr_global(ctx, I) { - if (I->dest[0].type != BI_INDEX_NORMAL) continue; if (!bi_instr_uses_helpers(I)) continue; bi_foreach_src(I, s) { @@ -260,9 +263,15 @@ bi_analyze_helper_requirements(bi_context *ctx) bi_foreach_instr_global(ctx, I) { if (!bi_has_skip_bit(I->op)) continue; - if (I->dest[0].type != BI_INDEX_NORMAL) continue; - I->skip = !BITSET_TEST(deps, bi_get_node(I->dest[0])); + bool exec = false; + + bi_foreach_dest(I, d) { + if (I->dest[d].type == BI_INDEX_NORMAL) + exec |= BITSET_TEST(deps, bi_get_node(I->dest[d])); + } + + I->skip = !exec; } free(deps);
