-
Notifications
You must be signed in to change notification settings - Fork 45
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
Hint Mode + again #193
Comments
to the file where the IncrementWait Cava circuit is defined, splitting up a two-field parameter record along the way to avoid having to define more parameter rewrapping instances, running into a nasty usability issue (mit-plv/bedrock2#193) caused by Hint Mode + on map.map.
I don't suppose setting a mode of |
This is the part that seems wrong/surprising to me. It'd be great if we got coqdev advice on whether this is intended and/or how to avoid it. Would "just use parameter records instead of word and map parameters" be an acceptable workaround in your use case? |
It does change something: It sends typeclass search into an infinite loop! I minimized the infinite loop to the following: Class word := { }.
Module MMIO.
Class parameters := {
word :> word;
}.
End MMIO.
Instance MMIO_compiler_params{word: word}: MMIO.parameters := {
MMIO.word := word;
}.
Check MMIO.word. (* loops for a few seconds, then stack overflow *) I'm surprised that Coq doesn't have any mechanism to avoid such loops. Is it my responsibility to set
I'll try to also isolate the original bug above.
I'll ask once I have isolated it.
No, because I'm still trying to get rid of parameter records, which are one of the biggest sources of user-unfriendliness (syntactically different implicit arguments cause |
It has a manual mechanism called
Generally, yes
Write out the typeclass hint graph and make sure it's a dag?
What's the typeclass debug log? It's probably not a bug, but we need to see the log to be sure. |
That only allows regular expressions to match branches to cut, but I'd like to cut all branches with a duplicate hint, but I guess that's not expressible as a regex.
The way we were intending to use parameter records keeps creating situations where it's not a DAG. Here's an example: Module Semantics.
Class parameters := {}.
End Semantics.
Module Lib1.
Class parameters := {
foo: nat;
semantics_params :> Semantics.parameters;
}.
End Lib1.
Module SubprojectWithFooEqual42.
Module CommonDefinitions.
Instance make_Lib1_params{p: Semantics.parameters}: Lib1.parameters := {
foo := 42;
semantics_params := p;
}.
End CommonDefinitions.
Module File1.
Section WithParams.
Context {p: Semantics.parameters}.
(* use definitions of Lib1... *)
End WithParams.
End File1.
Module File2.
Section WithParams.
Context {p: Semantics.parameters}.
(* use definitions of Lib1... *)
End WithParams.
End File2.
Module File3.
Section OoopsForgotToRequireParameters.
Fail Timeout 1 Check (_ : Semantics.parameters).
End OoopsForgotToRequireParameters.
End File3.
End SubprojectWithFooEqual42.
To break the loop, we'd have to stop sharing parameter record transformer instances like |
I guess |
Or maybe we should just ban |
On the other hand, one benefit of parameter records is that if several files take the same |
See coq/coq#14707 |
* sketch riscv machine model that concurrently runs a cava-defined device * sketch deterministic executable RISC-V machine with Cava device * we can now vm_compute the bedrock2-compiled IncrementWait example on the deterministic riscv-coq machine with a Cava increment circuit attached * beware, `Hint Extern` without a pattern can lead to stack overflow when running eg `Check (_ : Utility.Words)` * put StateMachineMMIOSpec into separate file so that MMIOToCava proof does not need to depend on MMIO compiler proof * sketch statement connecting the compiler's riscv machine to the riscv machine with a Cava device, and statement connecting a bedrock2 program to the riscv machine with a Cava device * update bedrock2 compiler to not using MemoryLayout any more * prove bedrock2_and_cava_system_correct (but the interesting part, stateMachine_to_cava_1, is still admitted) * wip porting IncrementWait to new constants style and generic StateMachineMMIO rather than IncrementWaitMMIO * manually revert IncrementWait to using separate register for status * document `device` typeclass * cava incr device with separate VALUE/STATUS registers * update IncrementWait proofs to generic StateMachineSemantics * adding the right cut brings Qed time from seemingly infinite down to ~1sec * replace `program` seplog assertion by `ptsto_bytes` of `instrencode` in `bedrock2_and_cava_system_correct`, because `program` contains "hidden" assumptions about the validity of the instructions emitted by the compiler, which we want to appear as a separate hypothesis, because the hypothesis "the memory contains the program" will not be discharged inside Coq, so it shouldn't contain any hidden surprises * include program logic soundness in bedrock2_and_cava_system_correct * prove end-to-end theorem for IncrementWait (but important parts like bridging between the compiler's RISC-V machine and the RISC-V machine with the Cava device are still admitted) * make sure machine_ok uses mmioAddr expressed in terms of lower layer (ExtraRiscvMachine) rather than intermediate layer (which is supposed to cancel out us much as possible) * instead of requiring that the Cava device remembers the latest request, pass the current request on the device's input wires * define execution of risc-v machine with cava device as interp of free monad (instead of directly using a Riscv monad instance), so that we can prove the correspondence between the compiler's risc-v machine and the cava risc-v machine for each Riscv primitive separately, without having to go through Run.run1 of all risc-v instructions. However, this requires mid-conditions implying existence of a state machine execution, and it's not clear how to make that condition inductive. Will probably have to require read_step/write_step in mmioLoad/mmioStore of MinimalMMIO, instead of relying on post asserting existence of an execution. * add MMIOWriteOK and assert it in nonmem_store, so that we get a `write_step` when inverting a MinimalMMIO risc-v machine execution without having to rely on an `execution` in the postcondition, which is not inductive * use the MaterializeRiscvProgram.Materialize instance instead of a metrics-specific instance (moving the metrics updating from the RiscvProgram instance to the interp function), so that the compiler's risc-v machine differs from the Cava risc-v machine only in its interp function, which allows us to reason about their equivalence at riscv_primitive granularity * finish stateMachine_to_cava (but still have to see if we can instantiate its assumptions, device_implements_state_machine) * moving the `device_implements_state_machine` instance for IncrementWait to the file where the IncrementWait Cava circuit is defined, splitting up a two-field parameter record along the way to avoid having to define more parameter rewrapping instances, running into a nasty usability issue (mit-plv/bedrock2#193) caused by Hint Mode + on map.map. * first sketch of cava_counter_satisfies_state_machine, still several problems in the device_state_related relation, but already found and fixed two bugs in the Cava IncrementWait circuit: - when the result value is read by the software, the device has to go back to IDLE - the device must leave the IDLE state only if the request is a value write request, but stay in IDLE if the request is any other request * don't rely on uniqueness of state machine execution any more (but still require that it simulates a deterministic Cava device, so there's only as much room for nondeterminism in the state machine as allowed by the simulation relation) * simplification/fixes related to initial states, any_two_imply_the_third is not sufficient * initial state conditions seem to work * try to allow high-level underspecification nondeterminism in state_machine_read/write_to_device_read/write, which seems necessary to allow to not precisely specify in the high-level state machine how many times we need to poll until the device says that it went from BUSY to DONE. Problem: Can't prove any more that the (forall s, execution t s -> device_state_related s d) condition of mkRelated is preserved, but still need it, because we get one execution from the (exists s, execution t s /\ device_state_related s d) part in mkRelated for which we also get a device_state_related, and another execution from mmioLoad, for which we don't get a device_state_related, but need to construct one * using execution_unique, the MMIO machine to Cava machine proof works again, and still allows external high-level underspecification nondeterminism (but not nondeterminism that's not immediately visible in the trace), which is enough to prove that the Cava IncrementWait device implements the IncrementWait state machine. The IncrementWaitToCava end-to-end theorem is now almost admit-free: Bbesides the propositional & functional extensionality axioms, there are only three more admits, which all are uninteresting properties about words (`naive_riscv_ok`, `incrN_word_to_bv` and `MMIOToCava.bv_to_word_word_to_bv`). * cleanup
While working on silveroak, I got an error where
straightline_call
saysError: Cannot infer this placeholder of type "spec_of put_wait_get" (no type class instance found)
, andCheck (_ : spec_of put_wait_get)
andCheck (_ : spec_of "put_wait_get")
both agree, they can’t find any instance either.However, the following instance is available:
and it is registered as a typeclass instance and shows up in
Print HintDb typeclass_instances
asand if I do
I get
from which I conclude that Coq is also able to infer the missing arguments of
@spec_of_put_wait_get
, so overall, the typeclass search should succeed, so why does it fail??So why can
Check spec_of_put_wait_get
infer the missing arguments, whereasCheck (_: spec_of put_wait_get)
cannot?Using
Set Typeclasses Debug Verbosity 2
and comparing the output of the two, I think I found out why:Check spec_of_put_wait_get
first looks for aword 32
, finds(@MMIO.word p)
, and then looks for a(map.map (@MMIO.word p) byte)
, which succeeds as well.On the other hand,
Check (_: spec_of put_wait_get)
starts by findingspec_of_put_wait_get
, which opens two subgoals, and since the first subgoal appears as a dependency in the second subgoal, the first subgoal is shelved, and typeclass search works on the second subgoal, which appears as(map.map ?word byte)
. Now, since we have aGlobal Hint Mode map.map + + : typeclass_instances
incoqutil.Map.Interface
, the two arguments ofmap.map
are treated as inputs to typeclass search, and since one of them is unknown, this subgoal is “suspended”, and resolution continues “on the remaining goals”, but there are none (because the shelved ones are not considered), so it tries one more time, and then sees that it has reached “a fixed point when the set of remaining suspended goals does not change”, so no solution is found.This explanation is derived from combining the output of
Set Typeclasses Debug Verbosity 2
with the explanation in this paragraph in the the manual:A simple solution to this problem is to simply do
Global Hint Mode map.map - - : typeclass_instances
, because then typeclass search succeeds on the(map.map ?word byte)
subgoal.I also tried
Set Typeclasses Dependency Order
, but that didn’t help.Another solution might be to redeclare each instance like
Instance spec_of_put_wait_get': spec_of "put_wait_get" := spec_of_put_wait_get.
, and to adapt the program logic Ltac to unfold all spec_of instances until they look like a spec rather than a redeclared instance, but that sounds cumbersome.The reason why this hasn’t shown up earlier is that all previous code either had all declarations and usages of
spec_of
instances in the sameSection
, so that the instances were not parameterized over map instances, or the map instances were inside a parameter record that got inferred as a whole, but as soon as aspec_of
instance declared in oneSection
depends on amap
instance and is needed in a different file orSection
, typeclass search will fail as described above.This is quite a bad usability issue, and I expect it to appear more often the more bedrock2 is used, especially when library functions are reused by different client functions.
Currently my only solution is
Global Hint Mode map.map - - : typeclass_instances
, but I’d be curious if you find another solution @andres-erbsen?The text was updated successfully, but these errors were encountered: