Slightly less nonsensical term equality test in progress tactical.#22064
Slightly less nonsensical term equality test in progress tactical.#22064ppedrot wants to merge 2 commits into
Conversation
For what seems to amount to historical reasons, the progress tactical considered that two unifiable sorts were equal and thus not resulting in a progress. This is very weird and unlike the corresponding treatment of instances. A quick blame indicates that the intended behaviour may have been to *check* quotient by universe unification through the evarmap, which is weaker than *enforcing* this quotient. This commit precisely does that and makes the sort code behave like the instance code.
|
You can remove the reference now that the evar_map is only read from. |
|
CI green, as expected nobody relies on that. |
|
Out of curiosity, let's @coqbot bench |
| else | ||
| try sigma := add_constraints !sigma UnivProblem.(Set.singleton (UEq (s1, s2))); true | ||
| with UGraph.UniverseInconsistency _ | UniversesDiffer -> false | ||
| else Sorts.equal (EConstr.ESorts.(kind sigma (make s1))) (EConstr.ESorts.(kind sigma (make s2))) |
There was a problem hiding this comment.
why do instances use ugraph aware comparison but this only checks the flexible instantiations?
There was a problem hiding this comment.
Indeed, you're right. For uniformity we should do the same in both cases, but it's not obvious to me which is the more reasonable choice. AFAICT ugraph-aware comparison is slightly more surprising because the printer still treats equivalent universes differently. (Not that I believe it matters a lot in practice, but once again having a spec for this low-level function should be important.)
|
🏁 Bench results: INFO: failed to install 🐢 Top 25 slow downs┌────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SLOW DOWNS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 63.9 66.2 2.3070 3.61% 608 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 63.5 65.4 1.9852 3.13% 608 coq-bedrock2/bedrock2/src/bedrock2Examples/lightbulb.v.html │ │ 43.9 45.1 1.2022 2.74% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/WithBedrock/fiat_crypto.v.html │ │ 34.7 35.7 0.9941 2.87% 194 coq-fiat-crypto-with-bedrock/src/Bedrock/P256/Jacobian.v.html │ │ 89.7 90.7 0.9580 1.07% 968 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 24.7 25.7 0.9329 3.77% 788 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 8.97 9.83 0.8613 9.61% 435 coq-mathcomp-odd-order/theories/PFsection12.v.html │ │ 44.0 44.8 0.7856 1.79% 3 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/bedrock2_fiat_crypto.v.html │ │ 42.3 43.1 0.7660 1.81% 2 coq-fiat-crypto-with-bedrock/src/ExtractionJsOfOCaml/fiat_crypto.v.html │ │ 23.6 24.3 0.6728 2.85% 782 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 21.4 22.1 0.6521 3.04% 49 coq-fiat-crypto-with-bedrock/src/Curves/Weierstrass/AffineProofs.v.html │ │ 22.1 22.7 0.5529 2.50% 776 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 107 107 0.5355 0.50% 22 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/ArithWithCasts.v.html │ │ 19.3 19.8 0.5209 2.70% 559 coq-mathcomp-odd-order/theories/PFsection9.v.html │ │ 18.1 18.6 0.4915 2.71% 32 coq-performance-tests-lite/src/pattern.v.html │ │ 17.9 18.3 0.4904 2.75% 571 coq-mathcomp-odd-order/theories/PFsection9.v.html │ │ 0.346 0.807 0.4605 132.99% 682 rocq-stdlib/theories/Numbers/DecimalFacts.v.html │ │ 16.5 16.9 0.4602 2.80% 762 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 89.9 90.3 0.4550 0.51% 999 coq-performance-tests-lite/src/fiat_crypto_via_setoid_rewrite_standalone.v.html │ │ 10.8 11.2 0.4053 3.77% 159 coq-fiat-crypto-with-bedrock/src/Bedrock/P256.v.html │ │ 7.86 8.27 0.4030 5.13% 1331 coq-mathcomp-odd-order/theories/PFsection9.v.html │ │ 0.451 0.848 0.3967 87.91% 393 coq-fiat-crypto-with-bedrock/src/Curves/Montgomery/XZProofs.v.html │ │ 20.5 20.9 0.3832 1.87% 24 coq-fiat-crypto-with-bedrock/src/Rewriter/Passes/MultiRetSplit.v.html │ │ 42.9 43.3 0.3750 0.87% 256 coq-fiat-crypto-with-bedrock/src/Bedrock/P256/Jacobian.v.html │ │ 23.7 24.1 0.3566 1.50% 13 coq-fourcolor/theories/proof/job295to298.v.html │ └────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ 🐇 Top 25 speed ups┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐ │ TOP 25 SPEED UPS │ │ │ │ OLD NEW DIFF %DIFF Ln FILE │ ├─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤ │ 201 199 -1.7399 -0.87% 8 coq-neural-net-interp-computed-lite/theories/MaxOfTwoNumbersSimpler/Computed/AllLogits.v.html │ │ 41.7 40.7 -1.0042 -2.41% 235 coq-category-theory/Construction/DecoratedCospan/Category.v.html │ │ 18.6 17.9 -0.7117 -3.82% 481 coq-verdi-raft/theories/RaftProofs/EndToEndLinearizability.v.html │ │ 28.9 28.3 -0.5852 -2.02% 305 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/Addchain.v.html │ │ 31.4 30.9 -0.5126 -1.63% 180 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 0.737 0.239 -0.4972 -67.49% 374 rocq-stdlib/theories/Sorting/SetoidList.v.html │ │ 31.2 30.7 -0.4960 -1.59% 198 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 31.3 30.8 -0.4833 -1.54% 157 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 31.2 30.8 -0.4780 -1.53% 139 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 31.2 30.8 -0.4728 -1.51% 166 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 31.5 31.0 -0.4382 -1.39% 148 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 39.4 39.0 -0.3887 -0.99% 1423 coq-fiat-crypto-with-bedrock/rupicola/bedrock2/compiler/src/compiler/FlatToRiscvFunctions.v.html │ │ 0.667 0.279 -0.3883 -58.22% 1 rocq-stdlib/theories/micromega/SatDivMod.v.html │ │ 1.03 0.647 -0.3838 -37.23% 160 rocq-stdlib/theories/Numbers/HexadecimalNat.v.html │ │ 21.6 21.2 -0.3812 -1.76% 651 rocq-stdlib/theories/Zmod/ZmodBase.v.html │ │ 32.5 32.1 -0.3794 -1.17% 121 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 31.3 30.9 -0.3400 -1.09% 214 coq-fiat-crypto-with-bedrock/src/Bedrock/Field/Synthesis/Examples/p224_64_new.v.html │ │ 54.4 54.0 -0.3346 -0.62% 296 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/Addchain.v.html │ │ 49.1 48.8 -0.3256 -0.66% 376 coq-unimath/UniMath/ModelCategories/Generated/LNWFSMonoidalStructure.v.html │ │ 0.615 0.291 -0.3239 -52.67% 110 rocq-stdlib/theories/Numbers/HexadecimalPos.v.html │ │ 7.27 6.95 -0.3209 -4.41% 35 coq-fiat-crypto-with-bedrock/src/PushButtonSynthesis/SolinasReductionReificationCache.v.html │ │ 0.312 0.0148 -0.2974 -95.25% 117 coq-mathcomp-analysis/theories/derive.v.html │ │ 34.3 34.0 -0.2824 -0.82% 898 coq-fiat-crypto-with-bedrock/src/Bedrock/Secp256k1/JoyeLadder.v.html │ │ 26.7 26.4 -0.2808 -1.05% 375 coq-unimath/UniMath/ModelCategories/Generated/LNWFSMonoidalStructure.v.html │ │ 42.9 42.6 -0.2808 -0.65% 221 coq-fiat-crypto-with-bedrock/src/Bedrock/P256/Coord32.v.html │ └─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘ |
|
I have one more argument in favour of equality rather than unification: the former is actually monotonous w.r.t. evarmap extension, while the latter isn't. Without this property, the fast path on defined evars from tclPROGRESS is actually unsound. |
|
For that fast path to be sound we would need equality ignoring definition of flexibles right? |
|
Formally we need for all terms c1, c2 and evarmaps σ1 ⊆ σ2, |
|
What's |
|
Syntactic equality i.e. without evar expansion. |
|
Anyway pick equality or equality up to ugraph and use it for both instances and sort, then this should be ready to merge IMO |
|
I would argue that the "correct" test is : if the quality variables or universe variables are both flexible, then they should be "indistinguishable", and if not they should be syntactically equal. Since we don't want to know whether flexible things are distinguishable, we have to wonder whether we assume they always are distinguishable (then, we go for syntactic equality) or indistinguishable (then we only test for flexibility and if not for syntactic equality). |
For what seems to amount to historical reasons, the progress tactical considered that two unifiable sorts were equal and thus not resulting in a progress. This is very weird and unlike the corresponding treatment of instances. A quick blame indicates that the intended behaviour may have been to check quotient by universe unification through the evarmap, which is weaker than enforcing this quotient. This commit precisely does that and makes the sort code behave like the instance code.