-
Notifications
You must be signed in to change notification settings - Fork 0
Write symbolic value at symbolic index #151
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
Conversation
src/kriscv/kdist/riscv-semantics/lemmas/sparse-bytes-simplifications.md
Outdated
Show resolved
Hide resolved
src/kriscv/kdist/riscv-semantics/lemmas/sparse-bytes-simplifications.md
Outdated
Show resolved
Hide resolved
…WB operations. Update test data to include symbolic index value handling in storeBytes. Adjust formatting for clarity.
…ing a boolean wrapper for symbolic writes. This update includes detailed explanations of termination control, reordering for optimization, and write consolidation strategies. Adjusted comments for clarity and improved documentation.
src/kriscv/kdist/riscv-semantics/lemmas/sparse-bytes-simplifications.md
Outdated
Show resolved
Hide resolved
rule #WB(true, I, V, NUM, EF:SparseBytesEF) => writeBytesEF(I, V, NUM, EF) [simplification, concrete(I)] | ||
|
||
// Reordering for Optimization | ||
rule #WB(false, I0, V0, NUM0, #WB(true, I1, V1, NUM1, B:SparseBytes)) => #WB(true, I1, V1, NUM1, #WB(false, I0, V0, NUM0, B)) [simplification] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a requires
clause missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need requires
clause there. It push uncompleted #WB
down to the completed one with true
as its first argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If so, why is it sound to reorder writes unconditionally, in particular, to the same index but with different values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the Write Consolidation
simplification rules have higher priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That unfortunately is not a mandatory directive for the backends: https://github.com/runtimeverification/k/blob/c829b1099ad225400bea5bcd5e9a21905060db06/docs/user_manual.md?plain=1#L1405-L1414
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I feel there might be some issues. If we add requires here, there could be situations where we cannot determine whether I0 and I1 are equal. This could lead to #WB that can never be completed. Because simplification rules also don't generate branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, I want this rule to work like owise
to tackle uninterpreted cases. Even though this cannot be the default case, we still need an owise to cover all situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rule #WB(false, I0, V0, NUM0, #WB(true, I1, V1, NUM1, B:SparseBytes)) => #WB(true, I1, V1, NUM1, #WB(false, I0, V0, NUM0, B)) [simplification] | |
rule #WB(false, I0, V0, NUM0, #WB(true, I1, V1, NUM1, B:SparseBytes)) => #WB(true, I1, V1, NUM1, #WB(false, I0, V0, NUM0, B)) [simplification] | |
requires I0 =/=Int I1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on slack, the rule here is probably unsound when the written bytes overlap.
Example:
#WB(false, 0, 65535, 2, #WB( true, 1, 0, 1, bytes(b"\x00\x00")))
I can evaluate this directly to #WB(false, 0, 65535, 2, bytes(b"\x00\x00"))
and then bytes(b"\xFF\xFF")
.
However, I could apply the simplification and get
#WB( true, 1, 0, 1, #WB(false, 0, 65535, 2, bytes(b"\x00\x00")))
but that would be #WB(true, 1, 0, 1, bytes(b"\xFF\xFF"))
and then bytes(b"\xFF\x00")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the suggested constraints on this rule.
…termination control, reordering for optimization, and write consolidation strategies. Update code examples for better understanding and consistency.
… Update documentation to emphasize the reordering optimization and the specific rule for merging operations with identical indices. Improve code examples for better clarity.
…ed the placement of the `requires` clause for clarity and consistency in simplification rules.
Hi @tothtamas28! I tried adding the |
…ration rule for better handling of overlapping writes. Update test data to ensure no overlapping writes between different indices, improving clarity and correctness in the specifications.
…s. Update documentation to clarify the edge case for identical indices, ensuring operations are marked complete to prevent infinite rewriting cycles. Adjust simplification annotation for improved clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM.
I left a few suggestions for improvements, but not necessarily on this same PR, could be done later if there are good reasons to merge this quickly.
I leave the approval to @tothtamas28 as I am unfamiliar with the surrounding code.
(4 <=Int I0 orBool I0 +Int 4 <=Int 0) andBool | ||
(4 <=Int I1 orBool I1 +Int 4 <=Int 0) andBool | ||
(4 <=Int I2 orBool I2 +Int 4 <=Int 0) andBool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not very common to have so many orBool
in the path condition.
Maybe you can make a few more tests, positive and negative (e.g. one like the counterexample I had) and replace the orBool
conditions by more tests with one condition each?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
src/kriscv/kdist/riscv-semantics/lemmas/sparse-bytes-simplifications.md
Outdated
Show resolved
Hide resolved
The rule below handles a termination edge case: it immediately marks the operation as complete with reduced priority to avoid infinite rewriting cycles. | ||
|
||
```k | ||
rule #WB(false, I0, V0, NUM0, #WB(true, I1, V1, NUM1, B:SparseBytes)) => #WB(true, I0, V0, NUM0, #WB(true, I1, V1, NUM1, B)) [simplification(60)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule might fire if the one above is not applied for random reasons, for instance SMT solver timeout. However, it will just cut the simplification short so it is sound.
…to ensure truly non-overlapping writes. Update comments for improved clarity on index constraints and their relationships.
Co-authored-by: Jost Berthold <jost.berthold@gmail.com>
…-counter specification to clarify handling of perfect overlapping writes. Ensure that only the covering write is retained in the output, enhancing clarity on index relationships.
No description provided.