Skip to content
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

Open
samuelgruetter opened this issue Jul 20, 2021 · 9 comments
Open

Hint Mode + again #193

samuelgruetter opened this issue Jul 20, 2021 · 9 comments

Comments

@samuelgruetter
Copy link
Contributor

While working on silveroak, I got an error where straightline_call says Error: Cannot infer this placeholder of type "spec_of put_wait_get" (no type class instance found), and Check (_ : spec_of put_wait_get) and Check (_ : spec_of "put_wait_get") both agree, they can’t find any instance either.

However, the following instance is available:

spec_of_put_wait_get : forall {word : word 32}, map.map word byte -> spec_of "put_wait_get"

and it is registered as a typeclass instance and shows up in Print HintDb typeclass_instances as

For spec_of ->   simple eapply @spec_of_put_wait_get(level 3, pattern spec_of "put_wait_get", id 0) 

and if I do

Set Printing Implicit.
Check spec_of_put_wait_get.

I get

@spec_of_put_wait_get (@MMIO.word p) (@MMIO.mem p)
     : spec_of "put_wait_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, whereas Check (_: 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 a word 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 finding spec_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 a Global Hint Mode map.map + + : typeclass_instances in coqutil.Map.Interface, the two arguments of map.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:

The mode hints (see Hint Mode) associated with a class are taken into account by typeclasses eauto. When a goal does not match any of the declared modes for its head (if any), instead of failing like eauto, the goal is suspended and resolution proceeds on the remaining goals. If after one run of resolution, there remains suspended goals, resolution is launched against on them, until it reaches a fixed point when the set of remaining suspended goals does not change.

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 same Section, 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 a spec_of instance declared in one Section depends on a map instance and is needed in a different file or Section, 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?

samuelgruetter added a commit to samuelgruetter/silveroak that referenced this issue Jul 20, 2021
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.
@JasonGross
Copy link
Contributor

I don't suppose setting a mode of - for word changes anything? I think this might also be worth a bug report on Coq?

@andres-erbsen
Copy link
Contributor

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

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?

@samuelgruetter
Copy link
Contributor Author

I don't suppose setting a mode of - for word changes anything?

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 Hint Mode or other flags appropriately in the code base to make sure no loops happen? If so, do you have a systematic approach to convince yourself that a codebase is typeclass-search-loop free? Or is this a Coq bug?

I think this might also be worth a bug report on Coq?

I'll try to also isolate the original bug above.

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.

I'll ask once I have isolated it.

Would "just use parameter records instead of word and map parameters" be an acceptable workaround in your use case?

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 rewrite, lia, ecancel_assumption and many other tactics to stop working).

@JasonGross
Copy link
Contributor

I'm surprised that Coq doesn't have any mechanism to avoid such loops.

It has a manual mechanism called Hint Cut

Is it my responsibility to set [...] flags appropriately in the code base to make sure no loops happen?

Generally, yes

If so, do you have a systematic approach to convince yourself that a codebase is typeclass-search-loop free?

Write out the typeclass hint graph and make sure it's a dag?

Or is this a Coq bug?

What's the typeclass debug log? It's probably not a bug, but we need to see the log to be sure.

@samuelgruetter
Copy link
Contributor Author

It has a manual mechanism called Hint Cut

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.

Write out the typeclass hint graph and make sure it's a dag?

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 make_Lib1_params, and each file would have to redeclare all of them. This would lead to boilerplate explosion and even more syntactically mismatching implicit arguments. Or is there a better way to do parameter records in this example?

@samuelgruetter
Copy link
Contributor Author

I guess make_Lib1_params could be a Definition, and then I'd do Existing Instance make_Lib1_params in each section, but still not so thrilled by it ...

@samuelgruetter
Copy link
Contributor Author

Or maybe we should just ban :> inside typeclass definitions, and instead of doing tc_field :> T, do Existing Instance tc_field but only inside sections. This would require a series of Existing Instance lines at the beginning of each section, but would maintain the invariant that Requireing a file never adds parameter record projections as typeclass hints, so in order to know all parameter record projections in the typeclass hints, it suffices to look at all Existing Instance commands of the current section.
And since in this setting, a loop always contains at least one projection, the local list of Existing Instance could serve as a starting point for checking for loops.

@samuelgruetter
Copy link
Contributor Author

On the other hand, one benefit of parameter records is that if several files take the same n arguments, we only need to spell them out once in the definition of a parameter record, instead of spelling them out at the beginning of each file, but if we require an Existing Instance command for each field which itself is a typeclass, that benefit is gone.

@samuelgruetter
Copy link
Contributor Author

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.

See coq/coq#14707

samuelgruetter added a commit to project-oak/silveroak that referenced this issue Jul 27, 2021
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants