fix[next]: Improve concat_where pruning - #2795
Conversation
|
One case from #2767 is not covered by this PR, and since #2767 is now closed I want to flag it before it gets lost.
symbolic_cond if arg == true_field else cond_complement
The result is a silent wrong answer, not a crash. Minimal reproducer, plain Cartesian, no connectivity: @gtx.field_operator
def testee(a: IKField) -> IKField:
t = a + 1.0
return concat_where(KDim < 2, t, t) # both branches equal, so the result is `t` everywhere
@gtx.program
def prog(a: IKField, out: IKField):
testee(a, out=out, domain={IDim: (0, 3), KDim: (0, 4)})with
So this is pre-existing on The fix is to select by position instead of by value: for is_true_branch, arg in [(True, true_field), (False, false_field)]:
...
symbolic_cond if is_true_branch else cond_complement,Applied to Otherwise the PR does what #2767 did, checked rather than read: #2767's regression test passes here on every backend and fails on |
havogt
left a comment
There was a problem hiding this comment.
lgtm, 2 structural comments. feel free to disagree. Might post an agent review as well in a bit.
havogt
left a comment
There was a problem hiding this comment.
Two issues in the new prune_empty_concat_where logic, both reproduced locally. The rest of the diff checks out - in particular the _infer_concat_where fix is a genuine latent bug (arg == true_field was true on both iterations when the branches were structurally equal, so identical branches both got the true-branch domain).
… helpers - pass the actual offset_provider and symbolic_domain_sizes from the pass manager into prune_empty_concat_where so the re-inference of a surviving equal branch works with unstructured shifts (previously crashed with an empty offset provider); regression test added - extract _broadcast_to and _node_with_explicit_broadcast as free functions
…concat_where_improvements
|
Unrelated to this PR, but noticed while looking at
elif cpm.is_call_to(node, "not_eq"):
# `IDim != a` -> `IDim < a & IDim > a`
return self.visit(
im.call("and_")(
With Only the compiled path is affected; This reproduces identically on The fix looks like one token, |
Problem
A
concat_whereimplicitly broadcasts each branch to the dimensions the branch does not have itself: inconcat_where(K < 0, a, b)witha: Field[[Vertex]]andb: Field[[Vertex, K]], accessed onVertex: [0, 10), K: [0, 10), the true branch is effectivelybroadcast(a, (Vertex, K)). This broadcast exists only implicitly — nothing in the IR represents it, and consequently the inferred domain of a branch is restricted to the dimensions of the branch's own type: the domain of the true_branch argumentais justVertex: [0, 10).prune_empty_concat_wheredecided whether a branch is ever selected from exactly these branch domains (filtered to the dimensions of the condition), which fails in two ways:ais selected whereK < 0, i.e. nowhere withinK: [0, 10)— but the empty rangeK: [0, 0)proving this is exactly the one missing froma's domain, soawas never pruned.concat_where(0 <= K, a, b)selectsaeverywhere and was pruned to plaina— replacing the two-dimensional expression by a one-dimensional one, as nothing accounted for the surviving branch relying on the implicit broadcast. This is a hard failure with statically known domain bounds: GTIR type inference later hitsassert expr_type.dims == target_type.dimsinvisit_SetAt(and underpython -O, the mismatched IR reaches the backend).Design
To decide whether a branch is ever selected, the pass makes the implicit broadcasts explicit (
broadcast(a, (Vertex, K))) and reruns domain inference on the resultingconcat_where. Since an explicitbroadcastspans all dimensions of the expression, nothing is dropped from its inferred domain, and the pruning decision reduces to a plainbranch.annex.domain.empty()check on that IR expression. Domain inference is used instead of computing the branch domains in the pass itself for simplicity and to not duplicate the domain semantics ofconcat_where. A survivingbroadcastbranch stays in the IR and is lowered by the subsequentRemoveBroadcastpass via the domain annex populated from the prunedconcat_where— so both bugs disappear with one mechanism. Branches that are equal (up to the implicit broadcast) are pruned directly; the surviving branch is reinferred on the full domain of theconcat_where, since its domains stem from the region where only its branch instance was selected.To make the re-inference cheap,
infer_domain.infer_exprgains arevisit_already_inferredoption (defaultTrue, preserving behavior): withFalse, subexpressions that already carry a domain are neither descended into nor altered, so only the newly createdbroadcastnodes are populated.Also in this PR:
im.broadcast(expr, dims)infer_exprtyped with a TypeVar so input and output expression types matchNew test cases
Notation:
concat_where(<cond>, <tb>: <tb dims>, <fb>: <fb dims>): <accessed domain> → <result>Behavior changes (fail or crash against
main's pass):Guard against the inverse mistake (a branch lacking the concat dimension is still selected via the implicit broadcast and must not be treated as never-selected):
Added coverage (already worked on
main):The unit test additionally asserts, for every case, that the result's domain annex equals the accessed domain (
RemoveBroadcastrelies on it), and, for prunedbroadcastresults, that the domains inside the branch are inferred from the full domain of theconcat_where.Integration: a test for the field branch with fewer dimensions than the expression was added to
test_concat_where.py:Since statically known domain bounds are required for the pruning to trigger — before this PR no test compiled with static bounds, so the pass had no end-to-end coverage at all — the test is parametrized on a new
static_domainsfixture:dynamic_domainsruns with runtime bounds as before, whilestatic_domainscreates the field operator with@gtx.field_operator(static_domains=True), making all field domains known at compile time so the pruning fires. The parametrization is extended to all tests in the file as it also improves coverage in general — in their static variant, the existing tests with never-selected branches now exercise the pass as well.AI disclaimer: This PR was written with the help of AI tools. The description and pass implementation were reviewed in detail. Tests were only briefly reviewed, by putting them into a simple unified format and working with them makes me confident for them to be meaningful.