Repository navigation
Fix shared cnf in withTemps under the new compiler - #43
Open
katjabercic wants to merge 2 commits into
Open
katjabercic wants to merge 2 commits into
katjabercic wants to merge 2 commits into
Conversation
Since Lean v4.22 the LCNF simplifier substitutes the s.vMap projection in EncCNF.withTemps to its use site after the inner encoder run, keeping the outer state (and its cnf) alive across the run. Every addClause inside a withTemps scope then saw a shared cnf and copied the whole array, once per scope. Extract the tempified state and vMap through a @[noinline] helper consumed before the run, and re-enable the Sinz #guard_msgs test that the resulting warnings had broken.
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.
Problem
Since the switch to the new code generator (Lean v4.22), builds on
devemite.g. 10× for the single
#eval!inTrestle/Encode/Cardinality/Sinz.lean, and the corresponding#guard_msgstest had to be disabled.Root cause
In
EncCNF.withTemps, the source readslet vMap := s.vMapbefore running the inner encoder so thatsdies early. The new LCNF simplifier treats the projection as a free-to-move value and substitutes it to its use site — after the inner run. The resulting IR doesinc s, runs the whole inner encoder, and only thenproj s.vMap; dec s. So the outer state, and hence itscnf, stays live across the entire inner run: the firstaddClausein everywithTempsscope sees a sharedcnfand copies the whole array. Since everytseitin[...]fragment opens its own scope, encodings degrade to one full CNF copy per fragment (quadratic overall).Fix
Extract the tempified state and the surviving
vMapin one step through a@[noinline]helper (LawfulState.withTempsAux) that is consumed before the inner run, and destructure its result withmatch. The compiler can then no longer move any read ofspast the run. Checked the compiled IR: the pair is projected and freed before the inner call, and nothing referencessafterwards.Two notes:
@[noinline] def vMapGet s := s.vMap) makes things worse: it compiles to apapcapturing the whole state, which then lives inside the restoredvMapforever — the same bug the!!!!!comment in this file describes. The surviving value must be a saturated object, hence the pair.C.map _ vMap→C.map vMap) thatdevcurrently needs to build at all; drop it if you already have your own version.Verification
Full
lake buildon v4.28.0: succeeds with zero sharing warnings (previously Sinz ×10, Tseitin ×3). The#guard_msgstest in Sinz.lean is re-enabled and passes. Isolated#evaltests of plainEncCNF,seq,for_all, nested/sequentialwithTempsare all silent.The underlying compiler behavior (projection substitution extending an object's lifetime past a call, defeating destructive updates) still reproduces on v4.32.2 and is probably worth an upstream lean4 issue; dumping the IR of a clone of
withTempswithset_option trace.compiler.ir.result truedemonstrates it directly.