Conversation
tensor_zero compared by hash alone, and the hash covers only the node id, so every zero of every shape was one key: three differently shaped zeros collapsed to a single map entry. Equality and ordering now compare dim and rank; the hash stays id-only, as a fast reject. Signed-off-by: petlenz <[email protected]>
The pass buckets candidates by argument hash, and hash(c*T) equals hash(T) by design, so the combining step re-checks the argument deeply. Nothing exercised that guard, and nothing calls the pass at all, so a future caller could have lost it silently. Signed-off-by: petlenz <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #453.
Part 1 —
tensor_zeroidentity ignores shape (real, fixed). Equality compared hashes, and the hash covers only the node id, so every zero of every shape was one key:0{3,2} == 0{2,4}set{0{3,2}, 0{2,4}, 0{3,4}}operator==now also compares dim and rank;operator<keeps the hash as a fast reject and falls through to(dim, rank).The hash is deliberately unchanged, following the precedent of #443/PR #458 for tensor symbols: the hash is a fast reject and an ordering key, not an identity, and changing it churns hash-driven print order (measured there: several print-pinned tests flip). Verified unchanged here:
pow(x,2)*y*Aprint order, and both invariants —tensor_scalar_mul(const,T).hash == T.hash,tensor_pow(T,const).hash == T.hash. All zero folds still work:A+0,0+A,A-0,0-A → -A,2*0 → 0{2},trace(0),det(0),norm(0).Part 2 — the issue's claim does not hold; no fix needed.
tensor_projector_simplifierdoes bucket candidates by argument hash, but the combining step already re-checks deeply:That guard came from
f92f94e("Fix #340, #341: replace raw hash-equality identity checks with deep comparison"), which the audit that filed this issue missed — it read the map declaration at line 112 but not the guard 27 lines later. Confirmed by probe:vol(A) + dev(2A)stays2*dev(A)+vol(A), whilevol(A) + dev(A)folds tosym(A). Bucketing by hash loses nothing, since equal arguments necessarily hash equal.Decision: keep the file, add the missing test. The issue offered "fix the keying or delete it"; the keying is already correct, so the only real choice was deletion. Against it: the class is described in
DOCUMENTATION.mdanddocs/simplifier-coverage.md, and the tensor rule-contract work has an open decision to either wire it in as the driven optional pass or drop it as redundant — deleting now preempts that, in the irreversible direction. What the issue actually worried about ("whoever wires it inherits the bug") is addressed byProjectorPassGroupsOnlyDeepEqualArguments, which is the first test to exercise this class at all: it asserts the aliasing still holds, then that a mixed-argument sum does not combine. It passes on main by design — a lock-in, not a bug demonstration, and the PR says so rather than implying it caught something.Singletons checked as the issue asked:
scalar_zero,scalar_one,tensor_to_scalar_zeroandtensor_to_scalar_oneall have default constructors and carry no dim/rank or other per-instance state (the t2s constants pre-insert assumptions, identical for every instance), so identity by node id alone remains correct for them.Tests:
HashIdentitySweep.TensorZeroShapeIsPartOfIdentity(equality, ordering totality, a 3-entry map) andHashIdentitySweep.ProjectorPassGroupsOnlyDeepEqualArguments.Negative control (
tensor_zero.hrestored from main withgit show, rebuilt): the zero test fails on 7 assertions and passes with the fix; the projector lock-in passes either way as intended. Full suite 2407/2407 (gcc-14 Debug), clang-format clean.