Refold before evar instantiation#19987
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
There are some slowdowns to be investigated, especially the metacoq one seems too huge to be acceptable. |
rocq-prover/rocq#19987 refolds terms before using them to instantiate evars. There is one instance where we need to unfold by hand.
|
MetaRocq/metarocq#1152 is only a small performance improvement, so I think we might as well retry the CI now. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
|
I expect the errors were due to me not rebasing. Can we try again? |
This comment was marked as resolved.
This comment was marked as resolved.
Until now, the algorithm unfolded terms blindly and instantiated an evar with whatever term was on the other side. Now, we remember the terms from the initial unification problem, and whenever we reach a problem of the form `?e = t`, we replace `t` with its initial version.
|
It seems the problem is always when we get stuck on a match on an evar, in which case it does not really make sense to refold the other term since we will have to reduce it anyway. So I think we can try again. |
|
May I ask for a bench? |
|
@coqbot bench |
|
🏁 Bench results: 🐢 Top 25 slow downs┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 63.2 64.4 1.1800 1.87% 609 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 35.3 36.2 0.8988 2.55% 974 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 25.4 26.2 0.8030 3.16% 12 coq-fourcolor/theories/proof/job499to502.v.html │ │ 29.3 30.0 0.6850 2.33% 974 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 45.3 46.0 0.6771 1.49% 118 coq-bedrock2/bedrock2/src/bedrock2Examples/full_mul.v.html │ │ 45.4 46.1 0.6471 1.43% 118 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/full_mul.v.html │ │ 99.2 99.9 0.6424 0.65% 968 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 99.0 99.6 0.6104 0.62% 999 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 2.33 2.94 0.6068 26.03% 1001 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 91.8 92.4 0.6004 0.65% 20 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/NBE.v.html │ │ 24.5 25.1 0.5726 2.34% 12 coq-fourcolor/theories/proof/job503to506.v.html │ │ 41.3 41.9 0.5610 1.36% 579 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/MMIO.v.html │ │ 0.649 1.14 0.4955 76.38% 813 rocq-stdlib/theories/MSets/MSetRBT.v.html │ │ 28.9 29.4 0.4929 1.70% 673 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 23.9 24.4 0.4900 2.05% 12 coq-fourcolor/theories/proof/job486to489.v.html │ │ 37.8 38.2 0.4674 1.24% 1423 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 47.5 47.9 0.4235 0.89% 376 coq-unimath/UniMath/ModelCategories/Generated/LNWFSMonoidalStructure.v.html │ │ 17.1 17.5 0.4059 2.38% 12 coq-fourcolor/theories/proof/job550to553.v.html │ │ 17.7 18.0 0.3716 2.10% 670 coq-performance-tests-lite/src/Nia.v.html │ │ 1.16 1.50 0.3401 29.23% 1142 rocq-stdlib/theories/FSets/FMapAVL.v.html │ │ 0.982 1.32 0.3360 34.23% 408 rocq-stdlib/theories/MSets/MSetAVL.v.html │ │ 11.9 12.2 0.2999 2.51% 194 coq-fiat-crypto-with-bedrock/src/Fancy/Barrett256.v.html │ │ 174 175 0.2976 0.17% 233 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/deps/riscv-coq/src/riscv/Proofs/DecodeByExtension.v.html │ │ 23.7 24.0 0.2906 1.23% 558 coq-bedrock2/bedrock2/src/bedrock2Examples/insertionsort.v.html │ │ 20.7 21.0 0.2844 1.37% 651 rocq-stdlib/theories/Zmod/ZmodBase.v.html │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 201 198 -2.7598 -1.37% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 65.5 64.0 -1.4792 -2.26% 609 coq-bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 79.4 78.2 -1.1861 -1.49% 48 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/AffineProofs.v.html │ │ 45.4 44.4 -0.9718 -2.14% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/WithBedrock/fiat_crypto.v.html │ │ 9.17 8.41 -0.7666 -8.36% 1331 coq-mathcomp-odd-order/theories/PFsection9.v.html │ │ 43.3 42.6 -0.7475 -1.73% 2 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/fiat_crypto.v.html │ │ 25.5 25.0 -0.5378 -2.11% 345 coq-fiat-crypto-with-bedrock/src/Curves/Montgomery/XZProofs.v.html │ │ 31.926 31.419 -0.5070 -1.59% 97 coq-vst/veric/binop_lemmas5.v.html │ │ 18.1 17.6 -0.4923 -2.72% 32 coq-performance-tests-lite/src/pattern.v.html │ │ 29.1 28.6 -0.4659 -1.60% 32 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/MontgomeryLadderRISCV.v.html │ │ 25.7 25.3 -0.4551 -1.77% 12 coq-fourcolor/theories/proof/job291to294.v.html │ │ 12.0 11.6 -0.4544 -3.78% 388 coq-unimath/UniMath/CategoryTheory/Hyperdoctrines/PartialEqRels/Logic/Existential.v.html │ │ 27.0 26.6 -0.4391 -1.63% 35 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/MontgomeryLadderRISCV.v.html │ │ 56.0 55.6 -0.4386 -0.78% 731 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JacobianCoZ.v.html │ │ 12.1 11.7 -0.4284 -3.54% 324 coq-unimath/UniMath/CategoryTheory/Hyperdoctrines/PartialEqRels/Logic/Existential.v.html │ │ 12.4 12.0 -0.4099 -3.31% 930 coq-unimath/UniMath/CategoryTheory/Hyperdoctrines/PartialEqRels/ExponentialEqs.v.html │ │ 58.5 58.1 -0.3983 -0.68% 27 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ToFancyWithCasts.v.html │ │ 0.700 0.319 -0.3814 -54.48% 12 rocq-stdlib/theories/MSets/MSets.v.html │ │ 28.0 27.7 -0.3719 -1.33% 12 coq-fourcolor/theories/proof/job107to164.v.html │ │ 44.9 44.5 -0.3663 -0.82% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/bedrock2_fiat_crypto.v.html │ │ 4.14 3.78 -0.3535 -8.55% 1190 coq-unimath/UniMath/CategoryTheory/GrothendieckConstruction/IsPullback.v.html │ │ 3.44 3.11 -0.3290 -9.56% 196 rocq-stdlib/theories/ZArith/ZModOffset.v.html │ │ 16.0 15.7 -0.3263 -2.04% 356 coq-fiat-crypto-with-bedrock/src/Bedrock/End2End/X25519/EdwardsXYZT.v.html │ │ 8.92 8.60 -0.3161 -3.55% 199 coq-mathcomp-odd-order/theories/PFsection12.v.html │ │ 7.66 7.34 -0.3151 -4.12% 602 coq-unimath/UniMath/CategoryTheory/EnrichedCats/Limits/Examples/StructureEnrichedLimits.v.html │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
|
Could someone take a look at this? |
|
@coqbot run full ci |
|
I think the code makes sense but I don't really like "heads" as the name for the original terms. Since the change broke some user code, I think it needs to have a changelog entry, and the PR description a comment on what/how breaks. I guess there is less unfolding in goals generated by tactics, but you surely know better. |
|
Would something like |
|
yes, I was thinking about |
|
@coqbot run full ci |
|
@coqbot merge now |
Until now, the algorithm unfolded terms blindly and instantiated an evar with whatever term was on the other side. Now, we remember the terms from the initial unification problem, and whenever we reach a problem of the form
?e = t, we replacetwith its initial version.This PR does the minimal change that changes the behavior of the unification algorithm as intended, but this makes the control flow a bit awkward and some computations are done twice, which is a bit inefficient. Should I try to be a bit more aggressive and rewrite some of the code?
Fixes / closes #????
make doc_gram_rsts.Overlays: