Extrude uniform parameters of inner fixpoints in guard condition check (grant #16040)#17986
Conversation
43d5435 to
d3e7d63
Compare
kernel/inductive.ml
Outdated
| let n = List.length l in | ||
| let n' = List.length l' in | ||
| if n < n' then List.map2 (&&) l (List.firstn n l') | ||
| else List.map2 (&&) (List.firstn n' l) l' |
There was a problem hiding this comment.
It's a weird way to write a function that can be written much more elegantly with a pattern-matching.
|
Contrarily to the other tweaks to the guard condition, I think that this one is indeed innocuous. There are some dubious places in the code but I don't understand it enough to predict the consequences. |
kernel/inductive.ml
Outdated
| if Int.equal n (nbodies + k - i) then | ||
| let args, extra = List.chop nargs l in | ||
| let new_args = snd (List.filter2 (fun uniform _ -> not uniform) argsign args) in | ||
| Term.applist (f, new_args @ extra) |
There was a problem hiding this comment.
This term could be ill-typed, couldn't it? How do we know that uniform parameters cannot depend on a non-uniform one? With SProp we lose injectivity of dependence on variables.
There was a problem hiding this comment.
You're right, as in:
Inductive sTrue : SProp := sI.
Parameter P : sTrue -> Prop.
Check fix f a (e : P a) n :=
match n with
| 0 => 0
| S n => f sI e n
end.And the same would typically also happen if we had η on unit.
Then we could either expect the uniform arguments to be a continuous prefix of the arguments, as that would cover the interesting cases, or to consider that definitionally-inhabited subsingleton arguments can always be extruded?
Characterizing the latter does not look so easy, so, by simplicity, I would go towards the first solution. Ok for you? (That means reducing the data argsign to just an int.)
There was a problem hiding this comment.
Actually, there is nothing wrong with extruding (a copy of) the definitionally-inhabited subsingleton arguments, or more generally with uniform arguments that do not constitute a prefix of the arguments. Moreover, the code already takes this into account. Assuming an interleaving of uniform γ and γ' and non-uniform δ as in:
fix f γ δ γ' n := φ(f γ δ' γ', ..., f γ δ'' γ')the translation is:
fun γ δ γ' => (fix f δ n := φ(f δ', ..., f δ'')) δWhat check_nested_fix_body is doing is to check the guard of φ(f δ', ..., f δ'') in the context γ δ γ' where γ and γ' inherit the size of their argument in the stack (if any). This implicitly means that it already considers not a fun γ γ' => ... extrusion (which might be ill-typed in general) but a fun γ δ γ' => ... δ combination of extrusion and expansion.
[Added 28/12] For the example above, the transformation is:
Check fun a (e : P a) => (fix f a n :=
match n with
| 0 => 0
| S n => f sI n
end) a.Note: I added a clarification in the code.
There was a problem hiding this comment.
I still fail to see why I (x, y, z) is a well-formed type in your comment. The z variable has not the right type as it has type C{x, y₀} rather than C{x, y} where y₀ is the outermost y variable.
There was a problem hiding this comment.
Regarding typing, I'm finding a small weakness in the algorithm: the arguments could be normalized before checking their uniformity: otherwise we may find u different of y but convertible to y, and fail to extrude it in a way which respects typing.
Assuming the normalization done, we know z:C{x, y} and z:C{x, u} from the original term and thus C{x, y} and C{x, u} convertible (or more precisely C{x, y} ≦ C{x, u} with subtyping). Since u is not convertible with y, then either y and u are absent in the first place or the convertibility proof of C{x, y} ≦ C{x, u} either eventually discards y and u, or makes them convertible from some other reason (e.g. being in an SProp). In any cases, the idea is that the convertibility proof of C{x, y} ≦ C{x, u} can be modified into a proof of convertibility of C{x, y₀} ≦ C{x, y} (using then your naming convention for y and y₀).
I don't think that missing an extrusion because of some u not detected as being convertible to the y is bad as we would just be more restrictive in the guard compared to what would give the optimal (well-typed) extrusion. However, do you think e.g. it would be better to check the convertibility of u with y (thus taking into account strict propness), rather than just reasoning on the form?
There was a problem hiding this comment.
either y and u are absent in the first place or the convertibility proof of C{x, y} ≦ C{x, u} either eventually discards y and u, or makes them convertible from some other reason
That doesn't really look like a formal proof to me. This kind of injectivity property is especially dubious in presence of SProp, we could end up proving UIP without realizing with this kind of feature.
There was a problem hiding this comment.
Would it be wrong to do a proof by induction on the structure and the reducts of C{x, _}? Then, am I missing other base cases than when C{_,_} does not depend on its second argument and when both C{x, y} and C{x, u} are detected proof irrelevant? (I'm asking because I'm not so familiar with the rules for proof irrelevance, so I may be missing some rules.)
More generally, is your intuition telling you that the result is wrong or is it that you'd like, as a rule, that statements about Coq TT are always formalized? In the latter case, I would need help because I don't have an infrastructure such as MetaCoq at hand to do a computer-checked proof.
There was a problem hiding this comment.
Alternatively, we can also renounce to non-prefix extrusion. This would still certainly cover most of the interesting cases.
There was a problem hiding this comment.
I'd prefer not to implement non-prefix extrusion so as to err on the safe side.
There was a problem hiding this comment.
Ok, done.
I also added more tests as well as found and fixed a residual bug (wrongly dropping the "NeedReduce" status of the uniform arguments of an inner fix by testing the need for reduction of only extra_stack in the previous version of the PR).
|
@herbelin if you want this merged you should undraft at some point |
|
I had missed @ppedrot's comment, sorry. I now answered. I wanted to add a few tests before undrafting. |
d3e7d63 to
db8af2e
Compare
db8af2e to
9dd2d32
Compare
Actually, the tests I had in mind were about a more ambitious version supporting uniform parameters across mutual fixpoints. That seems significantly more complicated (need to reason up to permutation of uniform arguments) and I renounced to it. |
|
🔴 CI failure at commit 9dd2d32 without any failure in the test-suite ✔️ Corresponding job for the base commit 2c068b8 succeeded ❔ Ask me to try to extract a minimal test case that can be added to the test-suite 🏃
|
|
If someone wants to have a second look. At least this looks ok on my side. (The fiat-crypto failure is probably related to upstream changes made yesterday.) |
|
cc @JasonGross |
|
Looks like the Fiat Crypto job is using an outdated rewriter artifact? If you rerun the ci-rewriter job then Fiat Crypto should succeed |
…ction". We do it at use time rather than artificially propagating it transitively to arguments that don't need reduction.
…x in guard checking. We proceed in three steps on the expansion of the relevant body of a block of fix: - first find the arguments of the recursive calls that are uniform - then drop those arguments - in addition to the main argument of the inner fix, we propagate the recursive specification of the arguments that are uniform towards the body of the fixpoints.
af7a654 to
1830851
Compare
|
🏁 Bench results: 🐢 Top 25 slow downs┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 2.3820 2.9110 0.5290 22.21% 487 coq-stdlib/Numbers/HexadecimalFacts.v.html │ │ 94.8160 95.3230 0.5070 0.53% 968 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 94.8030 95.2370 0.4340 0.46% 999 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 62.4270 62.8180 0.3910 0.63% 609 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 80.8680 81.2580 0.3900 0.48% 48 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/AffineProofs.v.html │ │ 47.2380 47.5880 0.3500 0.74% 558 coq-bedrock2/bedrock2/src/bedrock2Examples/insertionsort.v.html │ │ 241.6060 241.9550 0.3490 0.14% 141 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 0.5500 0.8700 0.3200 58.18% 736 coq-stdlib/Reals/Cauchy/ConstructiveCauchyReals.v.html │ │ 33.3230 33.6150 0.2920 0.88% 1333 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 6.4060 6.6730 0.2670 4.17% 3024 coq-fiat-crypto-with-bedrock/src/Assembly/EquivalenceProofs.v.html │ │ 25.4110 25.6720 0.2610 1.03% 345 coq-fiat-crypto-with-bedrock/src/Curves/Montgomery/XZProofs.v.html │ │ 0.0000 0.2550 0.2550 inf% 476 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/New/Signature.v.html │ │ 8.0550 8.3070 0.2520 3.13% 2854 coq-fiat-crypto-with-bedrock/src/Assembly/EquivalenceProofs.v.html │ │ 15.8430 16.0920 0.2490 1.57% 828 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/Jacobian/CoZ.v.html │ │ 36.4620 36.7060 0.2440 0.67% 548 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/MMIO.v.html │ │ 137.9620 138.1910 0.2290 0.17% 155 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 22.5150 22.7380 0.2230 0.99% 1073 coq-metacoq-safechecker/safechecker/theories/PCUICSafeReduce.v.html │ │ 7.3020 7.5220 0.2200 3.01% 604 coq-unimath/UniMath/CategoryTheory/EnrichedCats/Colimits/Examples/StructureEnrichedColimits.v.html │ │ 17.4150 17.6300 0.2150 1.23% 607 coq-mathcomp-odd-order/theories/PFsection9.v.html │ │ 0.5540 0.7680 0.2140 38.63% 422 coq-stdlib/MSets/MSetList.v.html │ │ 17.8010 18.0110 0.2100 1.18% 32 coq-performance-tests-lite/src/pattern.v.html │ │ 37.3950 37.6000 0.2050 0.55% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/bedrock2_fiat_crypto.v.html │ │ 8.0480 8.2470 0.1990 2.47% 1133 coq-metacoq-erasure/erasure/theories/ErasureFunction.v.html │ │ 0.5850 0.7780 0.1930 32.99% 816 coq-stdlib/MSets/MSetRBT.v.html │ │ 17.1780 17.3660 0.1880 1.09% 481 coq-verdi-raft/theories/RaftProofs/EndToEndLinearizability.v.html │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 182.1250 180.3930 -1.7320 -0.95% 233 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/DecodeByExtension.v.html │ │ 220.9070 219.5110 -1.3960 -0.63% 103 coq-fiat-crypto-with-bedrock/src/Arithmetic/BarrettReduction.v.html │ │ 155.8050 154.5760 -1.2290 -0.79% 1190 coq-unimath/UniMath/CategoryTheory/GrothendieckConstruction/IsPullback.v.html │ │ 257.5000 256.3240 -1.1760 -0.46% 1629 coq-metacoq-pcuic/pcuic/theories/PCUICSR.v.html │ │ 67.8680 66.9460 -0.9220 -1.36% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 3.9210 3.1790 -0.7420 -18.92% 153 coq-vst/veric/binop_lemmas2.v.html │ │ 257.6860 256.9680 -0.7180 -0.28% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 36.5180 35.9270 -0.5910 -1.62% 2 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/fiat_crypto.v.html │ │ 27.6420 27.1030 -0.5390 -1.95% 35 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/MontgomeryLadderRISCV.v.html │ │ 29.4820 28.9660 -0.5160 -1.75% 32 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/MontgomeryLadderRISCV.v.html │ │ 101.5690 101.0770 -0.4920 -0.48% 20 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/NBE.v.html │ │ 3.9240 3.4410 -0.4830 -12.31% 490 coq-stdlib/Reals/Cauchy/ConstructiveCauchyRealsMult.v.html │ │ 74.0610 73.5940 -0.4670 -0.63% 905 coq-unimath/UniMath/ModelCategories/Generated/LNWFSCocomplete.v.html │ │ 3.5090 3.1090 -0.4000 -11.40% 857 coq-vst/veric/expr_lemmas.v.html │ │ 24.6700 24.3510 -0.3190 -1.29% 49 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/AffineProofs.v.html │ │ 41.6970 41.3910 -0.3060 -0.73% 139 coq-fiat-parsers/src/Parsers/Refinement/SharpenedJSON.v.html │ │ 34.0720 33.7810 -0.2910 -0.85% 839 coq-unimath/UniMath/CategoryTheory/GrothendieckConstruction/IsPullback.v.html │ │ 62.6020 62.3130 -0.2890 -0.46% 609 coq-bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 25.6350 25.3570 -0.2780 -1.08% 24 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/MultiRetSplit.v.html │ │ 27.9450 27.6700 -0.2750 -0.98% 12 coq-fourcolor/theories/job563to588.v.html │ │ 131.8780 131.6030 -0.2750 -0.21% 22 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 0.8390 0.5650 -0.2740 -32.66% 200 coq-stdlib/Numbers/HexadecimalNat.v.html │ │ 18.1130 17.8410 -0.2720 -1.50% 12 coq-fourcolor/theories/job315to318.v.html │ │ 5.7200 5.4540 -0.2660 -4.65% 19 coq-fiat-crypto-with-bedrock/src/Language/IdentifiersBasicGENERATED.v.html │ │ 0.2540 0.0000 -0.2540 -100.00% 476 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/New/Signature.v.html │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
ppedrot
left a comment
There was a problem hiding this comment.
This change to the guard lies in the likely reasonable territory.
|
@coqbot merge now |
|
@ppedrot: You cannot merge this PR because:
|
|
@coqbot merge now |
…tch. Commit 140908d (PR rocq-prover#17986, about the extrusion of uniform parameters) was passing the wrong extra arguments of a match.
…tch. Commit 140908d (PR rocq-prover#17986, about the extrusion of uniform parameters) was passing the wrong extra arguments of a match.
…tch. Commit 140908d (PR rocq-prover#17986, about the extrusion of uniform parameters) was passing the wrong extra arguments of a match. (cherry picked from commit 1338fea)
As evidenced by the flurry of LLM-discovered soundness issues with the guard check, this commit was clearly not analyzed enough. In this commit we simply revert the part that introduced the uniform parameter support and leave the cleanups. It invalidates at least one concrete test from the test-suite, but the next commit should reinstall the same feature in a way that is much more robust.
The PR computes the parameters of inner fixpoints that are stable across recursive calls and extrudes them for the computation of guard through these inner fixpoints, thus granting the long-standing wish described in #16040.
Closes #16040