-
Notifications
You must be signed in to change notification settings - Fork 661
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
Slightly less crazy implementation of atom conversion in Firstorder #19126
Conversation
This indirection has the nice side-effect of not passing a crazy amount of time in comparison of terms when manipulating sequents. This is still not ideal because we keep normalizing terms before putting them in the table, but this is required for backwards compatibility.
@coqbot bench |
🏁 Bench results:
🐢 Top 25 slow downs┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 218.2370 220.7360 2.4990 1.15% 103 coq-fiat-crypto-with-bedrock/src/Arithmetic/BarrettReduction.v.html │ │ 179.9200 181.1480 1.2280 0.68% 233 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/DecodeByExtension.v.html │ │ 1.8830 2.8530 0.9700 51.51% 8134 coq-color/Coccinelle/term_orderings/rpo.v.html │ │ 25.6260 26.4560 0.8300 3.24% 129 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/Projective.v.html │ │ 1.4590 2.2860 0.8270 56.68% 8145 coq-color/Coccinelle/term_orderings/rpo.v.html │ │ 0.7670 1.5740 0.8070 105.22% 105 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Translation/Proofs/UsedVarnames.v.html │ │ 0.5990 1.4020 0.8030 134.06% 62 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/coqutil/src/coqutil/Word/Properties.v.html │ │ 0.5950 1.3620 0.7670 128.91% 62 coq-coqutil/src/coqutil/Word/Properties.v.html │ │ 235.4130 236.1040 0.6910 0.29% 141 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 0.7110 1.3970 0.6860 96.48% 98 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Translation/Proofs/UsedVarnames.v.html │ │ 24.3620 25.0460 0.6840 2.81% 12 coq-fourcolor/theories/job319to322.v.html │ │ 130.4140 131.0220 0.6080 0.47% 22 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 0.5890 1.1680 0.5790 98.30% 388 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Translation/Proofs/EquivalenceProperties.v.html │ │ 65.0810 65.5880 0.5070 0.78% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 0.6620 1.1110 0.4490 67.82% 385 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Translation/Proofs/EquivalenceProperties.v.html │ │ 48.7540 49.2010 0.4470 0.92% 558 coq-bedrock2/bedrock2/src/bedrock2Examples/insertionsort.v.html │ │ 86.5550 86.9730 0.4180 0.48% 365 coq-mathcomp-odd-order/theories/PFsection4.v.html │ │ 135.1070 135.5100 0.4030 0.30% 155 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 23.2590 23.6480 0.3890 1.67% 12 coq-fourcolor/theories/job554to562.v.html │ │ 17.5460 17.8690 0.3230 1.84% 32 coq-performance-tests-lite/src/pattern.v.html │ │ 0.3810 0.6880 0.3070 80.58% 2108 coq-stdlib/FSets/FMapFacts.v.html │ │ 28.9370 29.2210 0.2840 0.98% 12 coq-fourcolor/theories/job589to610.v.html │ │ 39.4910 39.7660 0.2750 0.70% 835 coq-fiat-crypto-with-bedrock/src/Fancy/Compiler.v.html │ │ 16.3810 16.6370 0.2560 1.56% 81 coq-fiat-crypto-with-bedrock/rupicola/src/Rupicola/Examples/Utf8/Utf8.v.html │ │ 11.7670 12.0110 0.2440 2.07% 527 coq-fiat-crypto-with-bedrock/src/AbstractInterpretation/Wf.v.html │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 256.2600 15.6940 -240.5660 -93.88% 1629 coq-metacoq-pcuic/pcuic/theories/PCUICSR.v.html │ │ 64.6910 61.2560 -3.4350 -5.31% 609 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 203.4010 200.9050 -2.4960 -1.23% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 3.5620 2.6350 -0.9270 -26.02% 3222 coq-metacoq-pcuic/pcuic/theories/PCUICConfluence.v.html │ │ 3.5570 2.6700 -0.8870 -24.94% 490 coq-stdlib/Reals/Cauchy/ConstructiveCauchyRealsMult.v.html │ │ 2.2440 1.4020 -0.8420 -37.52% 2995 coq-metacoq-template/common/theories/uGraph.v.html │ │ 3.4310 2.7230 -0.7080 -20.64% 3226 coq-metacoq-pcuic/pcuic/theories/PCUICConfluence.v.html │ │ 3.1860 2.4930 -0.6930 -21.75% 1295 coq-metacoq-pcuic/pcuic/theories/PCUICConversion.v.html │ │ 2.3120 1.6230 -0.6890 -29.80% 196 coq-stdlib/setoid_ring/Ncring_tac.v.html │ │ 3.1380 2.4510 -0.6870 -21.89% 1286 coq-metacoq-pcuic/pcuic/theories/PCUICConversion.v.html │ │ 5.4440 4.8220 -0.6220 -11.43% 52 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Common/Util.v.html │ │ 2.0470 1.4340 -0.6130 -29.95% 2996 coq-metacoq-template/common/theories/uGraph.v.html │ │ 82.6720 82.0830 -0.5890 -0.71% 48 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/AffineProofs.v.html │ │ 7.6780 7.1510 -0.5270 -6.86% 2813 coq-metacoq-pcuic/pcuic/theories/PCUICConfluence.v.html │ │ 3.0060 2.4920 -0.5140 -17.10% 3298 coq-metacoq-pcuic/pcuic/theories/PCUICConfluence.v.html │ │ 2.7730 2.2760 -0.4970 -17.92% 3281 coq-metacoq-pcuic/pcuic/theories/PCUICConfluence.v.html │ │ 6.7000 6.2730 -0.4270 -6.37% 626 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/LAN9250.v.html │ │ 0.6650 0.2590 -0.4060 -61.05% 525 coq-metacoq-template/common/theories/uGraph.v.html │ │ 96.0560 95.6540 -0.4020 -0.42% 999 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 1.4020 1.0060 -0.3960 -28.25% 854 coq-stdlib/FSets/FMapAVL.v.html │ │ 1.5630 1.1910 -0.3720 -23.80% 1250 coq-metacoq-erasure/erasure/theories/ErasureFunction.v.html │ │ 23.6520 23.2850 -0.3670 -1.55% 296 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/EdwardsXYZT.v.html │ │ 17.7300 17.3780 -0.3520 -1.99% 607 coq-mathcomp-odd-order/theories/PFsection9.v.html │ │ 0.5780 0.2720 -0.3060 -52.94% 319 coq-corn/metric2/FinEnum.v.html │ │ 6.4050 6.1030 -0.3020 -4.72% 2495 coq-iris-examples/theories/logatom/herlihy_wing_queue/hwq.v.html │ └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
The slowdown of some instances of firstorder is probably due to the fact that this PR fixes a bug, insofar as it uniformly strongly normalizes all atoms. For some reason, the And / Or trivial constructors only performed a weak-head reduction. This should go away in favour of a more reasonable approach anyways. |
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.
I don't know the code but the bench is very convincing.
@coqbot merge now |
The reification code of the firstorder tactic is completely bonkers, as it strongly normalizes (including δ!) all subterms appearing in a goal or hypothesis. We make it a tad less crazy by at least introducing an indirection so that normalized terms are only compared w.r.t. a unique identifier afterwards.
This should help in metacoq.