-
Notifications
You must be signed in to change notification settings - Fork 691
Use algebraic datatypes to signal failure in Conversion API #19120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use algebraic datatypes to signal failure in Conversion API #19120
Conversation
Just in case @coqbot bench |
let () = if irr_flex infos.cnv_inf fl1 then raise NotConvertible (* trigger the fallback *) in | ||
convert_stacks l2r infos lft1 lft2 v1 v2 cuniv | ||
with NotConvertible | UGraph.UniverseInconsistency _ -> | ||
with NotConvertible | NotConvertibleTrace _ -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't need to be in this PR, but in the case where neither fflex is unfoldable we may want to reraise this error instead of raising NotConvertible and dropping any further information.
🏁 Bench results:
INFO: failed to install coq-metacoq-template (dependency coq-equations failed) 🐢 Top 25 slow downs┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 76.3400 86.8600 10.5200 13.78% 365 coq-mathcomp-odd-order/theories/PFsection4.v.html │ │ 63.3860 72.0720 8.6860 13.70% 368 coq-mathcomp-odd-order/theories/PFsection4.v.html │ │ 217.7880 220.6330 2.8450 1.31% 103 coq-fiat-crypto-with-bedrock/src/Arithmetic/BarrettReduction.v.html │ │ 19.5350 22.1730 2.6380 13.50% 2061 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 180.6530 182.2260 1.5730 0.87% 233 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/DecodeByExtension.v.html │ │ 48.1390 49.5420 1.4030 2.91% 558 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/insertionsort.v.html │ │ 48.2520 49.5670 1.3150 2.73% 558 coq-bedrock2/bedrock2/src/bedrock2Examples/insertionsort.v.html │ │ 199.0940 200.3650 1.2710 0.64% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 16.1150 17.2930 1.1780 7.31% 828 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/Jacobian/CoZ.v.html │ │ 3.0180 3.9580 0.9400 31.15% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 16.9900 17.8350 0.8450 4.97% 607 coq-mathcomp-odd-order/theories/PFsection9.v.html │ │ 43.9300 44.6800 0.7500 1.71% 834 coq-vst/veric/binop_lemmas4.v.html │ │ 33.3670 34.1000 0.7330 2.20% 1333 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 37.6250 38.3510 0.7260 1.93% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/with_bedrock2_fiat_crypto.v.html │ │ 11.3020 11.9830 0.6810 6.03% 2103 coq-mathcomp-ssreflect/mathcomp/ssreflect/order.v.html │ │ 26.9450 27.4440 0.4990 1.85% 68 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/VerifyDecode.v.html │ │ 46.1320 46.6170 0.4850 1.05% 926 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/GarageDoor.v.html │ │ 36.1980 36.6780 0.4800 1.33% 2 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/fiat_crypto.v.html │ │ 37.5740 38.0520 0.4780 1.27% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/bedrock2_fiat_crypto.v.html │ │ 27.6570 28.0470 0.3900 1.41% 1933 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/RupicolaCrypto/ChaCha20.v.html │ │ 36.6770 37.0660 0.3890 1.06% 548 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/MMIO.v.html │ │ 32.8830 33.2420 0.3590 1.09% 97 coq-vst/veric/binop_lemmas5.v.html │ │ 21.4870 21.8370 0.3500 1.63% 376 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvMetric.v.html │ │ 14.9950 15.3210 0.3260 2.17% 1382 coq-rewriter/src/Rewriter/Language/Wf.v.html │ │ 29.7780 30.0880 0.3100 1.04% 194 coq-vst/veric/expr_lemmas4.v.html │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 64.8180 62.6220 -2.1960 -3.39% 609 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 64.1490 62.0200 -2.1290 -3.32% 609 coq-bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 101.3350 99.9840 -1.3510 -1.33% 20 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/NBE.v.html │ │ 40.2220 39.1510 -1.0710 -2.66% 236 coq-rewriter-perf-SuperFast/src/Rewriter/Rewriter/Examples/PerfTesting/LiftLetsMap.v.html │ │ 236.4230 235.4480 -0.9750 -0.41% 141 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 131.0580 130.2430 -0.8150 -0.62% 22 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 25.5340 24.8260 -0.7080 -2.77% 12 coq-fourcolor/theories/job227to230.v.html │ │ 47.0730 46.3740 -0.6990 -1.48% 110 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/full_mul.v.html │ │ 135.6380 134.9870 -0.6510 -0.48% 155 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 96.3010 95.7020 -0.5990 -0.62% 968 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 12.3470 11.7890 -0.5580 -4.52% 194 coq-fiat-crypto-with-bedrock/src/Fancy/Barrett256.v.html │ │ 96.2910 95.7480 -0.5430 -0.56% 999 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 38.7560 38.2210 -0.5350 -1.38% 224 coq-performance-tests-lite/PerformanceExperiments/rewrite_lift_lets_map.v.html │ │ 19.9760 19.4530 -0.5230 -2.62% 12 coq-fourcolor/theories/job507to510.v.html │ │ 4.8950 4.4030 -0.4920 -10.05% 1109 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Translation/Proofs/ValidComputable/Expr.v.html │ │ 30.0530 29.5720 -0.4810 -1.60% 12 coq-fourcolor/theories/job165to189.v.html │ │ 26.7330 26.2540 -0.4790 -1.79% 12 coq-fourcolor/theories/job399to438.v.html │ │ 25.2340 24.7880 -0.4460 -1.77% 12 coq-fourcolor/theories/job291to294.v.html │ │ 10.1480 9.7130 -0.4350 -4.29% 496 coq-rewriter/src/Rewriter/Rewriter/Wf.v.html │ │ 25.5440 25.1160 -0.4280 -1.68% 12 coq-fourcolor/theories/job499to502.v.html │ │ 20.1700 19.7460 -0.4240 -2.10% 214 coq-engine-bench-lite/coq/PerformanceDemos/one_step_reduction.v.html │ │ 24.2370 23.8250 -0.4120 -1.70% 12 coq-fourcolor/theories/job231to234.v.html │ │ 19.9120 19.5090 -0.4030 -2.02% 325 coq-engine-bench-lite/coq/PerformanceDemos/one_step_reduction.v.html │ │ 31.0300 30.6330 -0.3970 -1.28% 12 coq-fourcolor/theories/job254to270.v.html │ │ 26.5200 26.1320 -0.3880 -1.46% 12 coq-fourcolor/theories/job190to206.v.html │ └───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
Is this an actual slowdown? @silene claimed in another PR that the only column that mattered was the CPU instruction ones, which would mean that this is noise, but I'm a bit wary. The odd-order dev seems to be a bit noisy these days. |
To be clearer, it is the only column we can trust to be reproducible. The "user time" column matters too, but its results can be meaningfully interpreted only in |
The slow odd-order lines from the bench are really degenerated, and profiling them doesn't show any remarkable diff w.r.t. conversion. Rather, it's largely dominated by evar expansion, which is unrelated to this PR. |
The bench is -j1 on an idle machine (except coq-core and coq-stdlib which have bigger -j) |
I stand corrected. I hadn't understood that the various bench runners were running on physically distinct servers, and I had missed the |
In any case, the slight slowdown from odd-order will be made moot by #19123. These lines were hitting right in an algorithmic limitation of evar expansion, and after that PR they're 10 times faster or so (~80s to ~7s on my machine). |
This may generate slightly better error messages in some cases.
There are some dubious parts of the code where the NotConvertible exception is not properly caught, we leave a FIXME there to enhance the message at some future point.
Now all functions are returning algebraic errors so there is no point in exposing this internal detail.
We have to create local extensible types to do this in a type-safe way, but it should not matter for performance since this is only done once per toplevel invocation of conversion.
We only call fail_check from within eqappr rather than export wrappers that perform the call internally.
08c3e2f
to
79b9667
Compare
Let's rerun a bench in any case. @coqbot bench |
🏁 Bench results:
INFO: failed to install 🐢 Top 25 slow downs┌───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 256.1640 259.0930 2.9290 1.14% 1629 coq-metacoq-pcuic/pcuic/theories/PCUICSR.v.html │ │ 19.9930 21.8300 1.8370 9.19% 2061 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 181.3290 182.7070 1.3780 0.76% 233 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/DecodeByExtension.v.html │ │ 94.5390 95.8520 1.3130 1.39% 999 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 48.0640 49.2750 1.2110 2.52% 558 coq-bedrock2/bedrock2/src/bedrock2Examples/insertionsort.v.html │ │ 94.6500 95.8350 1.1850 1.25% 968 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 46.3850 47.3810 0.9960 2.15% 110 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/full_mul.v.html │ │ 129.6190 130.5790 0.9600 0.74% 22 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 46.3890 47.2440 0.8550 1.84% 110 coq-bedrock2/bedrock2/src/bedrock2Examples/full_mul.v.html │ │ 36.3530 37.1680 0.8150 2.24% 548 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/MMIO.v.html │ │ 33.3440 34.1070 0.7630 2.29% 1333 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 3.1110 3.8630 0.7520 24.17% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 11.8190 12.4230 0.6040 5.11% 194 coq-fiat-crypto-with-bedrock/src/Fancy/Barrett256.v.html │ │ 48.3770 48.9690 0.5920 1.22% 558 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/insertionsort.v.html │ │ 3.5890 4.1520 0.5630 15.69% 128 coq-fiat-parsers/src/Parsers/Refinement/SharpenedJSON.v.html │ │ 43.8430 44.3990 0.5560 1.27% 834 coq-vst/veric/binop_lemmas4.v.html │ │ 36.3070 36.8610 0.5540 1.53% 139 coq-fiat-parsers/src/Parsers/Refinement/SharpenedJSON.v.html │ │ 65.6070 66.1100 0.5030 0.77% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 30.5100 30.9730 0.4630 1.52% 12 coq-fourcolor/theories/job254to270.v.html │ │ 24.3320 24.7920 0.4600 1.89% 12 coq-fourcolor/theories/job503to506.v.html │ │ 19.8790 20.3390 0.4600 2.31% 214 coq-engine-bench-lite/coq/PerformanceDemos/one_step_reduction.v.html │ │ 29.6210 30.0620 0.4410 1.49% 12 coq-fourcolor/theories/job001to106.v.html │ │ 26.9110 27.3500 0.4390 1.63% 12 coq-fourcolor/theories/job287to290.v.html │ │ 26.2930 26.7230 0.4300 1.64% 12 coq-fourcolor/theories/job611to617.v.html │ │ 19.6220 20.0430 0.4210 2.15% 325 coq-engine-bench-lite/coq/PerformanceDemos/one_step_reduction.v.html │ └───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 86.2880 76.4550 -9.8330 -11.40% 365 coq-mathcomp-odd-order/theories/PFsection4.v.html │ │ 71.5770 63.4080 -8.1690 -11.41% 368 coq-mathcomp-odd-order/theories/PFsection4.v.html │ │ 202.5640 201.0450 -1.5190 -0.75% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 38.3980 37.3100 -1.0880 -2.83% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/with_bedrock2_fiat_crypto.v.html │ │ 10.5930 9.6990 -0.8940 -8.44% 496 coq-rewriter-perf-SuperFast/src/Rewriter/Rewriter/Wf.v.html │ │ 38.1500 37.3530 -0.7970 -2.09% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/bedrock2_fiat_crypto.v.html │ │ 23.5300 22.8610 -0.6690 -2.84% 296 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/EdwardsXYZT.v.html │ │ 10.4590 9.8240 -0.6350 -6.07% 496 coq-rewriter/src/Rewriter/Rewriter/Wf.v.html │ │ 36.7410 36.1210 -0.6200 -1.69% 2 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/fiat_crypto.v.html │ │ 4.9370 4.3460 -0.5910 -11.97% 1109 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Translation/Proofs/ValidComputable/Expr.v.html │ │ 27.6210 27.0440 -0.5770 -2.09% 68 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/VerifyDecode.v.html │ │ 11.7700 11.2290 -0.5410 -4.60% 2103 coq-mathcomp-ssreflect/mathcomp/ssreflect/order.v.html │ │ 100.2900 99.7560 -0.5340 -0.53% 20 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/NBE.v.html │ │ 27.4140 26.9750 -0.4390 -1.60% 35 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/MontgomeryLadderRISCV.v.html │ │ 29.1290 28.7260 -0.4030 -1.38% 32 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/MontgomeryLadderRISCV.v.html │ │ 16.1130 15.7250 -0.3880 -2.41% 828 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/Jacobian/CoZ.v.html │ │ 23.7380 23.4220 -0.3160 -1.33% 6 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/DecodeEncodeI.v.html │ │ 3.9000 3.5980 -0.3020 -7.74% 884 coq-mathcomp-odd-order/theories/BGsection16.v.html │ │ 4.0120 3.7180 -0.2940 -7.33% 441 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/GarageDoor.v.html │ │ 0.6600 0.3700 -0.2900 -43.94% 868 coq-stdlib/MSets/MSetRBT.v.html │ │ 18.2030 17.9170 -0.2860 -1.57% 708 coq-fiat-crypto-with-bedrock/src/Rewriter/RulesProofs.v.html │ │ 15.2960 15.0140 -0.2820 -1.84% 613 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlattenExpr.v.html │ │ 4.7350 4.4570 -0.2780 -5.87% 5 coq-fiat-crypto-with-bedrock/src/Assembly/Parse/Examples/fiat_p256_mul_optimised_seed11.v.html │ │ 7.5240 7.2540 -0.2700 -3.59% 356 coq-rewriter-perf-SuperFast/src/Rewriter/Rewriter/Wf.v.html │ │ 12.9110 12.6660 -0.2450 -1.90% 1555 coq-fiat-crypto-with-bedrock/src/Util/FSets/FMapTrie.v.html │ └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
No more oddorder slowdown, this may have been noise after all. |
@coqbot merge now |
@SkySkimmer: Please take care of the following overlays:
|
Instead of haphazardly raising exceptions on a unit type and hoping that somebody will eventually catch them, we make the kernel conversion function return a parameterized result type that additionally indicates the ways in which the function may fail.
As a a result, I found at least two bugs in Typeops where a stray NotConvertible exception could have reached toplevel without being caught when given ill-typed terms.
This should also alleviate the hassle of the upper layers where it's never clear which part of the code is supposed to handle the variouse universe failures in universe unification.
Overlays: