fix: a series of small mgmt & lattigo bugs - #3351
Open
AlexanderViand wants to merge 1 commit into
Open
Conversation
* Mirror Lattigo's FindBestBSGSRatio exactly for sparse diagonal sets
findBestBSGSRatio guarded the ratio comparison with nbN1 > 0 and compared in
integers. Lattigo divides in floating point without that guard, so a sparse
diagonal set, where nbN1 reaches 0 with nbN2 > 0, must take the > maxRatio
branch on the resulting +inf (and continue on a NaN 0/0). Guarding skips those
iterations and can select a different baby-step size than the backend, which
then generates rotation keys for the wrong Galois elements. Dense diagonal sets
never reach nbN1 == 0, so this only shows up for sparse transforms such as an
expanded convolution filter.
* Encode plaintexts at the requested slot count
Encoding at the ring's full slot count makes every ciphertext full-width, which
silently changes what a rotation means: rotations are modulo the slot count, so
a matrix packed at the requested (smaller) width lands in the wrong slots. On a
convolutional model that shows up as a ~5-orders-of-magnitude accuracy loss
(2.9e-02 relative error against 6.2e-07) rather than a failure.
The requested slot count is what the layouts packed to, so encode to that and
leave the ring as large as the parameters need it. The diagonals of a linear
transform are then narrower than the ring, which the verifier now allows: the
transform acts on the leading slots.
* Record the packing width before the bootstrap bump
Lattigo bootstrapping needs LogN >= 14, and since the ring dimension follows
from the slot count, the pass bumps minSlotCount to 8192 to get there. That
bump is about the ring, but it was also written back as the requested slot
count, which is what the layouts packed to and what plaintexts are encoded at.
Widening it desyncs the packed layouts from the ciphertexts.
Keep the pre-bump value for the requested-slot-count attribute.
* Require a linear operand for the kernel ops when placing relinearization
A linear transform is a bundle of rotations under Galois keys, and a Chebyshev
evaluation multiplies its input, so both need a linear ciphertext just as
tensor_ext.rotate does. Without that constraint the model may defer a
relinearization past them, leaving it applied to an already-linear result:
lattigo then fails with "ctIn.Degree() should be 2 but is 1".
This only becomes reachable once linear transforms survive to the backend.
* Only bootstrap ciphertexts
The bootstrap target walk filtered values by secret type, which does not
exclude a plaintext: a plaintext has no level for the analysis to track, that
reads as "needs bootstrap", and the pass then put a mgmt.bootstrap on an
mgmt.init. Nothing lowers that, and it surfaces far away as a ct-ct multiply
against a plaintext operand ('ckks.mul' op operand #0 must be
lwe-ciphertext-like).
Ask the secretness analysis as well.
* Charge lattigo's bootstrap no compute levels
Lattigo runs its bootstrapping circuit in its own parameter set, whose ring
carries extra primes: bootstrapping.NewParametersFromLiteral takes the compute
parameters as the *residual* ones. A bootstrap therefore returns the ciphertext
at the top of the compute chain and consumes none of it. Probing the emitted Go
for tcn confirms it: Bootstrap(in=41) returns out=26 with maxlevel=26.
Charging 16 levels made every level after a bootstrap 16 too low, which both
inflated the modulus chain and mis-scheduled the rescales that follow.
The budget the pass works from is levelBudget + bootstrapLevelsConsumed, so
a test that stated its budget net of the old 16-level charge now has to name
the total it meant: if.mlir asks for 18 where it used to ask for 2.
relu_composite now also inserts a real bootstrap, so its e2e test accepts
the bootstrapping evaluator and covers the correspondingly slower path.
loop_invariant_hoist_reproducer instead keeps the old charge as an explicit
override: its module was captured off tcresnet8small under a 16-level
charge, and reaching the loop with the levels exhausted is a precondition of
the hoisting it asserts rather than a property of lattigo.
* Bootstrap a copy so the operand survives
lattigo's Bootstrap mod-ups its argument into the bootstrapping ring in place
and leaves it there, which a probe on the emitted Go shows plainly:
boot BEFORE: ct4 lvl=1 log2scale=45
boot AFTER : ct4 lvl=29 log2scale=52 <- argument clobbered
out ct92 lvl=14 log2scale=45 <- result is fine
The op is a function of its operand, though, so the operand may still be live,
and a composite ReLU makes that the common case: x * step(x) needs the
un-bootstrapped x alongside the refreshed branch that produced step(x). On tcn,
7 of 28 bootstrap operands are read again afterwards - already at the CKKS
level, before any in-place assignment - so those reads take a ciphertext that
is no longer in the compute ring. The values are wrong from there on, and the
scale it leaves behind (2^52 against a base of 2^45) propagates until a later
bootstrap fails lattigo's message-ratio check:
panic: cannot BootstrapMany: initial Q/Scale = 7.999996
< 0.5*Q[0]/MessageRatio = 256.000000
Hand lattigo a copy. A ciphertext copy is negligible next to a bootstrap, so
this is unconditional rather than guarded on the operand having later uses.
This is the convention the dialect already uses for a value-semantics op whose
lattigo call is in-place: rescale_new, drop_level_new and negate_new all copy
the input and hand the copy to the in-place method. bootstrap was the one that
passed its operand straight in. Keeping the op Pure is right - the copy is what
makes it true - and it lets CSE deduplicate bootstraps of the same value, which
is worth having for the most expensive op in the program.
The op description still says it "applies bootstrapping in-place", which now
describes the lattigo call rather than the op; worth a separate docs pass.
* Model the level a lattigo chebyshev consumes
In the lattigo dialect every ciphertext has the same opaque type, so an
analysis learns a value's level only from the interfaces an op declares.
ckks.chebyshev declared none and was therefore modelled as consuming nothing,
while it consumes one level per level of the binary evaluation tree. Nothing
checks that belief, so the error is silent: alloc-to-inplace hands a
chebyshev's exhausted buffer to a later op as its destination, the backend
evaluates into it at min(operands, destination), and the quietly truncated
result surfaces much later as "cannot Rescale: input Ciphertext level is too
low". Declare ReducesLevelOpInterface, mirroring the depth
kernel.eval_chebyshev already models.
ckks.linear_transform deliberately does NOT declare it, and the op
description now says why: the transform is built at the input ciphertext's
own level, so evaluating it consumes no level, and the rescale the lowering
emits straight afterwards is the single level the multiplication costs.
Charging the transform as well double-counts, which drifts every value
downstream of a transform one level too deep until the next bootstrap resets
it, and makes the alloc-to-inplace level comparison unsound in a way the pass
cannot see.
* Do not assert in getRotationIndices on a bufferized diagonals operand
CKKSLinearTransformOp::getRotationIndices cast the diagonals operand to
RankedTensorType. The operand is a tensor before bufferization and a memref
after it, and the accessor is reachable in both states, so on an
already-bufferized module the cast aborts:
Casting.h:560: cast<To = mlir::RankedTensorType, From = mlir::ShapedType>:
Assertion `isa<To>(Val)' failed
The full pipeline runs configure-crypto-context before bufferization, so this
does not fire during a normal compile. It fires when the analysis is run on a
dumped module, which is how rotation-key collection gets debugged. Match on
ShapedType, which is all the accessor needs.
AlexanderViand
force-pushed
the
alex/small-fixes
branch
from
August 24, 2026 05:12
fe276b3 to
cfeeb0a
Compare
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.
A series of small fixes, mostly around Lattigo and related mgmt/etc that seemed too small to warrant individual PRs (but I'm happy to split this up if needed)
Summaries/Explanations (🤖 generated, sorry) of the changes below:
Mirror Lattigo's FindBestBSGSRatio exactly for sparse diagonal sets
findBestBSGSRatio guarded the ratio comparison with nbN1 > 0 and compared in
integers. Lattigo divides in floating point without that guard, so a sparse
diagonal set, where nbN1 reaches 0 with nbN2 > 0, must take the > maxRatio
branch on the resulting +inf (and continue on a NaN 0/0). Guarding skips those
iterations and can select a different baby-step size than the backend, which
then generates rotation keys for the wrong Galois elements. Dense diagonal sets
never reach nbN1 == 0, so this only shows up for sparse transforms such as an
expanded convolution filter.
Record the packing width before the bootstrap bump
Lattigo bootstrapping needs LogN >= 14, and since the ring dimension follows
from the slot count, the pass bumps minSlotCount to 8192 to get there. That
bump is about the ring, but it was also written back as the requested slot
count, which is what the layouts packed to and what plaintexts are encoded at.
Widening it desyncs the packed layouts from the ciphertexts.
Keep the pre-bump value for the requested-slot-count attribute.
Require a linear operand for the kernel ops when placing relinearization
A linear transform is a bundle of rotations under Galois keys, and a Chebyshev
evaluation multiplies its input, so both need a linear ciphertext just as
tensor_ext.rotate does. Without that constraint the model may defer a
relinearization past them, leaving it applied to an already-linear result:
lattigo then fails with "ctIn.Degree() should be 2 but is 1".
This only becomes reachable once linear transforms survive to the backend.
Only bootstrap ciphertexts
The bootstrap target walk filtered values by secret type, which does not
exclude a plaintext: a plaintext has no level for the analysis to track, that
reads as "needs bootstrap", and the pass then put a mgmt.bootstrap on an
mgmt.init. Nothing lowers that, and it surfaces far away as a ct-ct multiply
against a plaintext operand ('ckks.mul' op operand #0 must be
lwe-ciphertext-like).
Ask the secretness analysis as well.
Charge lattigo's bootstrap no compute levels
Lattigo runs its bootstrapping circuit in its own parameter set, whose ring
carries extra primes: bootstrapping.NewParametersFromLiteral takes the compute
parameters as the residual ones. A bootstrap therefore returns the ciphertext
at the top of the compute chain and consumes none of it. Probing the emitted Go
for tcn confirms it: Bootstrap(in=41) returns out=26 with maxlevel=26.
Charging 16 levels made every level after a bootstrap 16 too low, which both
inflated the modulus chain and mis-scheduled the rescales that follow.
The budget the pass works from is levelBudget + bootstrapLevelsConsumed, so
a test that stated its budget net of the old 16-level charge now has to name
the total it meant: if.mlir asks for 18 where it used to ask for 2.
relu_composite now also inserts a real bootstrap, so its e2e test accepts
the bootstrapping evaluator and covers the correspondingly slower path.
loop_invariant_hoist_reproducer instead keeps the old charge as an explicit
override: its module was captured off tcresnet8small under a 16-level
charge, and reaching the loop with the levels exhausted is a precondition of
the hoisting it asserts rather than a property of lattigo.
Bootstrap a copy so the operand survives
lattigo's Bootstrap mod-ups its argument into the bootstrapping ring in place
and leaves it there, which a probe on the emitted Go shows plainly:
The op is a function of its operand, though, so the operand may still be live,
and a composite ReLU makes that the common case: x * step(x) needs the
un-bootstrapped x alongside the refreshed branch that produced step(x). On tcn,
7 of 28 bootstrap operands are read again afterwards - already at the CKKS
level, before any in-place assignment - so those reads take a ciphertext that
is no longer in the compute ring. The values are wrong from there on, and the
scale it leaves behind (2^52 against a base of 2^45) propagates until a later
bootstrap fails lattigo's message-ratio check:
Hand lattigo a copy. A ciphertext copy is negligible next to a bootstrap, so
this is unconditional rather than guarded on the operand having later uses.
This is the convention the dialect already uses for a value-semantics op whose
lattigo call is in-place: rescale_new, drop_level_new and negate_new all copy
the input and hand the copy to the in-place method. bootstrap was the one that
passed its operand straight in. Keeping the op Pure is right - the copy is what
makes it true - and it lets CSE deduplicate bootstraps of the same value, which
is worth having for the most expensive op in the program.
The op description still says it "applies bootstrapping in-place", which now
describes the lattigo call rather than the op; worth a separate docs pass.
Model the level a lattigo chebyshev consumes
In the lattigo dialect every ciphertext has the same opaque type, so an
analysis learns a value's level only from the interfaces an op declares.
ckks.chebyshev declared none and was therefore modelled as consuming nothing,
while it consumes one level per level of the binary evaluation tree. Nothing
checks that belief, so the error is silent: alloc-to-inplace hands a
chebyshev's exhausted buffer to a later op as its destination, the backend
evaluates into it at min(operands, destination), and the quietly truncated
result surfaces much later as "cannot Rescale: input Ciphertext level is too
low". Declare ReducesLevelOpInterface, mirroring the depth
kernel.eval_chebyshev already models.
ckks.linear_transform deliberately does NOT declare it, and the op
description now says why: the transform is built at the input ciphertext's
own level, so evaluating it consumes no level, and the rescale the lowering
emits straight afterwards is the single level the multiplication costs.
Charging the transform as well double-counts, which drifts every value
downstream of a transform one level too deep until the next bootstrap resets
it, and makes the alloc-to-inplace level comparison unsound in a way the pass
cannot see.
Do not assert in getRotationIndices on a bufferized diagonals operand
CKKSLinearTransformOp::getRotationIndices cast the diagonals operand to
RankedTensorType. The operand is a tensor before bufferization and a memref
after it, and the accessor is reachable in both states, so on an
already-bufferized module the cast aborts:
The full pipeline runs configure-crypto-context before bufferization, so this
does not fire during a normal compile. It fires when the analysis is run on a
dumped module, which is how rotation-key collection gets debugged. Match on
ShapedType, which is all the accessor needs.