[Interp] Deferred Conflicting Assignments Checking, Scheduler Code Cleanup, and More Tests#165
[Interp] Deferred Conflicting Assignments Checking, Scheduler Code Cleanup, and More Tests#165
Conversation
There was a problem hiding this comment.
This looks good to me, great job! Just to confirm I'm understanding correctly: this PR replaces #157, where:
- #157 checked for conflicts eagerly at assignment time
- this PR delays this check until the end of each cycle, when all threads finish executing
And just to check: the assignment semantics you formalized in https://github.com/cucapra/protocols-paper/pull/6/changes reflect the behavior implemented in this PR and not #157, right? I'm fairly certain the "final state equivalence" property you mentioned in the writeup refers to this PR, but I wanted to double check -- thanks!
|
I also just added a commit with some extra doc-strings to help me better understand what's going on + a |
Technically, I think it applies to both. Most of the "proofs" in that write-up assume that there are no conflicting assignments. How we determine if a conflicting assignment occurs is not relevant under that assumption. Whether you find them statically, dynamically, right when they happen, or never, in this implementation doesn't affect the proof because the proof assumes a "correct/conflict-free" transaction sequence where a "correct" transaction sequence is one free of such conflicting assignment errors. I think calling the thing i wrote a formalization of the assignment semantics probably is a misnomer on my part. |
| p.resetn := 1'b0; | ||
| step(); | ||
| fork(); | ||
| p.resetn := 1'b1; // NOTE: this line is new! |
There was a problem hiding this comment.
Can we add a comment explaining why we do this?
| @@ -0,0 +1,55 @@ | |||
| // This file is identical to `pcpi_mul.prot`, except for minor changes | |||
| // (see comments below) | |||
There was a problem hiding this comment.
It would be nice to state here what this is testing.
| │ | ||
| 7 │ DUT.a := DUT.s; | ||
| │ ^^^^^ Cannot assign to forbidden input 'a' after observing a combinationally dependent output | ||
| │ ^^^^^ Cannot observe forbidden output 's' after assigning DontCare to a combinationally dependent input |
There was a problem hiding this comment.
This error message could probably be improved by identifying the name of the input and by inverting the causality. I.e., something like: output "s" depends on input "?" which does not have an assigned value.
ekiwi
left a comment
There was a problem hiding this comment.
I left some comments that would be worth going through. However, none of them are a blocker for merging this in. Great job, Nikil!
This PR changes how the scheduler and interpreter handle conflicting assignments in multi-threaded protocol execution. Previously (like in the last PR lol) the system checked for conflicts eagerly at the moment each assignment was evaluated. I found an example where this doesn't work. I changed this to a deferred model where conflicts are only checked once at the end of each cycle, after all threads have finished executing. This change fixes incorrect behavior where valid protocol compositions were being rejected, and also improves error reporting by showing the locations of both conflicting assignments.
The Problem with Eager Conflict Checking
To understand why eager conflict checking was problematic, consider the
wait_and_add_correct.txtest case. This test runs two transactions concurrently:The
wait_and_addprotocol waits for two cycles before setting the adder inputs, while theaddprotocol sets inputs immediately. Here's the relevant portion ofwait_and_add:And here's the
addprotocol that gets forked:The critical issue arises due to how input values are implicitly reapplied at the start of each cycle. When a thread doesn't explicitly assign to an input pin, the system reapplies whatever value that thread last assigned to preserve continuity. In cycle 2, the
addthread assignsa = 4andb = 5. Meanwhile,wait_and_addhasn't assigned anything toaorbyet in its execution, so when we enter cycle 2, the system implicitly reapplies the previous values forwait_and_add's inputs.With eager conflict checking, this implicit reapplication happened before
wait_and_addhad a chance to execute itsDUT.a := aassignment. The implicit reapplication would store a "don't care" or old value for thread 0, and then whenadd(thread 1) assigneda = 4, the system would compare these values and potentially flag a conflict, even thoughwait_and_addwas about to overwrite its implicit value with an explicit assignment.The fix is to defer conflict checking until after all threads have executed their statements for the cycle. By the time I check for conflicts,
wait_and_addhas executedDUT.a := 1andaddhas executedDUT.a := X, so things are good to go.To implement this, I removed the conflict checking logic from
apply_input_value, which is called during assignment evaluation. Instead, I added a new methodcheck_for_conflicts()that iterates through all input pins and examines the per-thread values stored for each once all threads have completed for the cycle.Finalizing Input Values
After checking for conflicts, I call
finalize_inputs_for_cycle()to apply the correct final values to the DUT simulation. This step is necessary because of how DontCare (X) assignments interact with eager value application.Consider the
wait_and_add_correct.txexample again. In cycle 2, after the fork:wait_and_add) executes:DUT.a := 1; DUT.b := 2;add) executes:DUT.a := X; DUT.b := X;During execution, assignments are applied eagerly to the simulation. But here's the problem: a DontCare assignment doesn't actually do anything to the simulation state (if other threads have assigned a concrete value for that input pin). It just records in the per-thread tracking that this thread no longer cares about this input's value. The simulation itself retains whatever value it had before.
This becomes problematic when you consider what might have happened earlier. At the start of cycle 2, thread 1 had concrete values (
a = 4,b = 5) from its execution in cycle 1. These values were eagerly applied to the simulation during the implicit reapplication phase. Now when thread 1 executesDUT.a := X, its per-thread value changes to DontCare, but the simulation still holdsa = 4from that earlier eager application.At the end of the cycle, the per-thread values are:
a = 1,b = 2(concrete values)a = X,b = X(DontCare)No conflict here since only one thread has concrete values. The correct final value for
ashould be1(the only concrete value). But the simulation might still havea = 4from thread 1's earlier assignment that it later "changed its mind" about.You might think we could handle this during execution: when we see a DontCare assignment, check if any other thread has a concrete value and use that. But this doesn't work because we don't check for conflicts until the end of the cycle. At the point when thread 1 assigns
X, thread 0 might not have executed yet, or thread 0 might have an intermediate value that it will later overwrite. We'd need foresight to know which concrete value will be the "real" final one, and making an arbitrary choice could apply a value that later turns out to conflict. So I think finalizing the input one last time to the sim at the end of all threads execution is the cleanest way to handle this, and is semantically correct (because in the view of any one transaction, the only time an input to the sim would "change" during this finalization stage is if it was DontCare anyways and doesnt matter to that transaction -- otherwise we'd have a conflicting assignment error).Code Cleanup surrounding Threads
There was some logic regarding the termination of threads and the clearing of the inputs for that thread that was overly complex in the previous PR. I fixed this in a few ways, mainly I moved the thread cleanup logic (clearing thread inputs and handling implicit forks) to occur after
finalize_inputs_for_cycle(). This ensures that input values are properly committed to the DUT before any cleanup occurs.Improved Error Messages
For conflicting assignment errors, I modified the diagnostic system to show both threads' assignment locations. Previously, error deduplication would suppress the second error because both used the same
StmtIdas the deduplication key. I added a newStmtKeyWithThread(StmtId, usize)variant to theErrorKeyenum, allowing the system to distinguish between errors for different threads even when they reference the same statement. The newemit_diagnostic_stmt_for_thread()function uses this thread-aware key for deduplication.Testing
The
wait_and_add_correct.txtest now correctly passes (returns 0) instead of incorrectly failing with a conflicting assignment error. Similarly, other tests involving concurrent protocols with overlapping but non-conflicting assignments now behave correctly. Tests that genuinely have conflicts, likeno_dontcare_conflict, now show error messages for both involved threads rather than just one. Also, added back thepcpi_mul_no_resetexample which now gets a nice MaxItersError.