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
[stdlib] Add Proper
instances for some number theorems to improve rewriting with inequalities
#16792
base: master
Are you sure you want to change the base?
Conversation
I tried out your example but it doesn't seem to be working...? |
@ana-borges Hm, maybe we should write |
@ana-borges It should work now. I also added a |
@haansn08 Cool! Could you add a changelog entry please? Also, what about mixing |
Proper
instances for some number theorems to improve rewriting with inequalities
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.
Please don't add new global instances. This goes against modularity.
@ppedrot I would like to make them |
Obviously, you have to import the corresponding modules for the instances to be available. Marking them global lets you do that silently through Require, but you lose any control over the fact that the instances are there (or not). |
@ppedrot I have now changed the visibility to |
Indeed, we need a serious set of rules for adding instances to the stdlib, so that users know when and what to import. I'm afraid that these guidelines do not exist yet... |
The way to fix this is to add |
@JasonGross Thanks for the tip! I tried putting the following at the end of (** Re-export [Proper] instances for rewriting *)
#[export] Existing Instances
_add_lt_mono_l
_add_lt_mono_r
_add_lt_mono
_add_le_mono
_add_le_lt_mono
_sub_le_mono_l
_sub_le_mono_r
_mul_lt_mono
_mul_le_mono
_pow_le_mono_l
_pred_le_mono
_le_lt_trans
_lt_le_trans
_log2_le_mono
_log2_up_le_mono
_sqrt_le_mono
_sqrt_up_le_mono. Also |
It occurs to me that there's an even better strategy, though. You can use filtered |
@ppedrot The initial example works now without any of the instances declared as @JasonGross I'm not quite sure how a filtered export could work, but it works using |
I'm not sure if this is what you're asking, but typeclass instances are just hints in the |
@JasonGross It might work, but I'm not sure how to do it. For now, |
8396f20
to
f89c9ce
Compare
The combination of all
|
@mrhaandi I will gladly remove those instances, but could you explain why this would lead to poor performance? |
Having every possible combination of |
For sure it is a waste to have instances like
|
@coqbot bench |
🏁 Bench results:
INFO: failed to install coq-category-theory 🐢 Top 25 slow downs
🐇 Top 25 speed ups
|
I now removed the |
@coqbot bench |
🏁 Bench results:
INFO: failed to install coq-category-theory 🐢 Top 25 slow downs
🐇 Top 25 speed ups
|
@coqbot run full ci |
1e18223
to
827aba6
Compare
@JasonGross I now squashed the commits. Is there anything else I can do to improve this PR? |
@coq/stdlib-maintainers please make a decision instead of leaving this open for years |
It is indeed a shame that this has been neglected so long. Who would be the most appropriate reviewer wrt considerations of using setoid rewriting at scale? I personally feel compelled to defer to @JasonGross, but I know he doesn't have all that much extra time on his hands these days and perhaps there's somebody who's actually in charge of setoid rewrite and associated infrastructure? @coq/typeclasses-maintainers @coq/tactics-maintainers ? |
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 think the optimal proper instances have eq in all argument slots but one, but these are good and there's no reason to block this PR. Can someone rebase and let's check full CI and bench again, and if everything looks fine let's merge
The PR rebeases cleanly but I get an error on push. Might it be that I am missing yet another type of permission, or would the PR author have to do it? |
827aba6
to
569b187
Compare
I tensed through the web interface by temporarily enabling it @coqbot bench |
🏁 Bench results:
🐢 Top 25 slow downs┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 46.3070 48.4560 2.1490 4.64% 110 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/full_mul.v.html │ │ 9.2280 10.5260 1.2980 14.07% 192 coq-vst/veric/binop_lemmas5.v.html │ │ 3.9250 4.9120 0.9870 25.15% 22 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 3.1550 3.9500 0.7950 25.20% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 95.2040 95.9170 0.7130 0.75% 999 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 133.0190 133.7230 0.7040 0.53% 22 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 2.6550 3.3200 0.6650 25.05% 34 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 1.6890 2.2880 0.5990 35.46% 196 coq-stdlib/setoid_ring/Ncring_tac.v.html │ │ 5.5590 6.1200 0.5610 10.09% 308 coq-iris-examples/theories/logrel/F_mu_ref_conc/binary/examples/stack/refinement.v.html │ │ 81.9700 82.5100 0.5400 0.66% 48 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/AffineProofs.v.html │ │ 2.0430 2.5730 0.5300 25.94% 32 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/NBE.v.html │ │ 11.2450 11.7350 0.4900 4.36% 126 coq-vst/veric/binop_lemmas6.v.html │ │ 154.1910 154.6720 0.4810 0.31% 1190 coq-unimath/UniMath/CategoryTheory/GrothendieckConstruction/IsPullback.v.html │ │ 1.6820 2.1060 0.4240 25.21% 42 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 72.5460 72.9690 0.4230 0.58% 905 coq-unimath/UniMath/ModelCategories/Generated/LNWFSCocomplete.v.html │ │ 87.7090 88.1280 0.4190 0.48% 365 coq-mathcomp-odd-order/theories/PFsection4.v.html │ │ 4.6420 5.0530 0.4110 8.85% 111 coq-bedrock2/bedrock2/src/bedrock2Examples/full_mul.v.html │ │ 1.6300 2.0150 0.3850 23.62% 20 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/NBE.v.html │ │ 1.6350 1.9710 0.3360 20.55% 165 coq-fiat-crypto-with-bedrock/src/Curves/Montgomery/XZProofs.v.html │ │ 14.6290 14.9640 0.3350 2.29% 490 coq-unimath/UniMath/HomologicalAlgebra/KA.v.html │ │ 0.6990 1.0240 0.3250 46.49% 813 coq-stdlib/MSets/MSetRBT.v.html │ │ 95.4300 95.7510 0.3210 0.34% 968 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 19.3540 19.6730 0.3190 1.65% 12 coq-fourcolor/theories/job546to549.v.html │ │ 39.6380 39.9450 0.3070 0.77% 835 coq-fiat-crypto-with-bedrock/src/Fancy/Compiler.v.html │ │ 23.8020 24.1020 0.3000 1.26% 12 coq-fourcolor/theories/job319to322.v.html │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 217.6290 216.3060 -1.3230 -0.61% 103 coq-fiat-crypto-with-bedrock/src/Arithmetic/BarrettReduction.v.html │ │ 63.3410 62.1350 -1.2060 -1.90% 609 coq-bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 66.6920 66.1310 -0.5610 -0.84% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 101.4420 101.0240 -0.4180 -0.41% 20 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/NBE.v.html │ │ 25.9170 25.5770 -0.3400 -1.31% 345 coq-fiat-crypto-with-bedrock/src/Curves/Montgomery/XZProofs.v.html │ │ 180.2030 179.8660 -0.3370 -0.19% 233 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/DecodeByExtension.v.html │ │ 18.5320 18.2040 -0.3280 -1.77% 820 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/Jacobian/CoZ.v.html │ │ 250.2350 249.9260 -0.3090 -0.12% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 38.0680 37.7660 -0.3020 -0.79% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/with_bedrock2_fiat_crypto.v.html │ │ 37.0070 36.7070 -0.3000 -0.81% 548 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/MMIO.v.html │ │ 18.1770 17.8790 -0.2980 -1.64% 708 coq-fiat-crypto-with-bedrock/src/Rewriter/RulesProofs.v.html │ │ 39.8510 39.5550 -0.2960 -0.74% 236 coq-rewriter/src/Rewriter/Rewriter/Examples/PerfTesting/LiftLetsMap.v.html │ │ 24.7550 24.4900 -0.2650 -1.07% 12 coq-fourcolor/theories/job291to294.v.html │ │ 18.3600 18.0980 -0.2620 -1.43% 32 coq-performance-tests-lite/src/pattern.v.html │ │ 16.1230 15.8630 -0.2600 -1.61% 828 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/Jacobian/CoZ.v.html │ │ 27.6420 27.3840 -0.2580 -0.93% 12 coq-fourcolor/theories/job107to164.v.html │ │ 23.2590 23.0110 -0.2480 -1.07% 12 coq-fourcolor/theories/job295to298.v.html │ │ 0.2770 0.0290 -0.2480 -89.53% 1104 coq-fiat-crypto-with-bedrock/src/CLI.v.html │ │ 0.2480 0.0000 -0.2480 -100.00% 476 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/New/Signature.v.html │ │ 0.2570 0.0130 -0.2440 -94.94% 518 coq-fiat-crypto-with-bedrock/src/CLI.v.html │ │ 135.0070 134.7650 -0.2420 -0.18% 155 coq-fiat-crypto-with-bedrock/src/UnsaturatedSolinasHeuristics/Tests.v.html │ │ 22.9820 22.7410 -0.2410 -1.05% 12 coq-fourcolor/theories/job554to562.v.html │ │ 0.2370 0.0000 -0.2370 -100.00% 65 coq-fiat-crypto-with-bedrock/src/Bedrock/Standalone/StandaloneOCamlMain.v.html │ │ 0.2350 0.0000 -0.2350 -100.00% 64 coq-fiat-crypto-with-bedrock/src/Bedrock/Standalone/StandaloneHaskellMain.v.html │ │ 7.3210 7.0880 -0.2330 -3.18% 604 coq-unimath/UniMath/CategoryTheory/EnrichedCats/Colimits/Examples/StructureEnrichedColimits.v.html │ └──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
@coqbot run full ci |
hb failure is unrelated ( |
I intend to merge on Monday or when I next look at this PR. |
This should add the required
Proper
instances to userewrite
like this: