Conversation
HadrienRenaud
left a comment
There was a problem hiding this comment.
A lot of work into this, nice one. I have a lot of small esthetics comments, feel free to choose the ones you want to keep.
Do you think there are some performance difference? For example on make vmsa-test? How long does make pac-test takes to run?
The main question I have is the following. If I understand correctly, right now the way to add a predicate is the following:
- Use an operation that use a collision wrapper
- This splits the execution and adds an equation
Assign (fresh_var (), Binop (AssumeCollision, v1, v2))(resp.AssumeNoCollisionon the other execution) (skipping the case where both values are constants) - When in
valconstraint.mlv1andv2are reduced to constantsc1 = {x with pac = p1}andc2 = {x with pac = p2},Constant.collisionreturnsSome (p1, p2), and thusAArch64Op.assumeCollisionraises aConstraint (Eq (p1, p2))(respNeq) valconstraint.mlthen pushes on the equation listPredicate (Eq (p1, p2))valconstraint.mlcalls the PAC solver at the end of its run with all the predicates accumulated.
Would it possible to skip the binop phase and directly push the predicates in AArch64Sem? That would change:
- similar
- This splits the execution and adds the predicates
AssumeCollision (v1, v2)(resp.AssumeNoCollision) valconstraintdoesn't touch the predicatesvalconstraintdoesn't touch the predicates- similar as before, but with
AssumeNoCollisioninstead ofNeqandAssumeCollisioninstead ofEq. Potentially we need to add a step of substitution inside predicates to convertAssumeCollision (V.Var s1, V.Var s2)intoAssumeCollision (V.Val c1, V.Val c2), but this is a very standard step.
The main advantage with this is that it would remove the need for the Constraint exceptions, and probably simplify the interaction between the PAC solver and the non-pac solver.
Please note that I don't know how litmus nor herd/constraints.ml work and so I can't really review this part of the code. I also haven't reviewed the litmus tests.
| if V.equal x y | ||
| then pure () st | ||
| else contradiction st |
There was a problem hiding this comment.
Extract this in a separate function?
fsestini
left a comment
There was a problem hiding this comment.
Thanks for the work you put into this. I'm not too familiar with the PAC feature, or the logic around constraints, so please bear with me if some of my comments on those aspects don't quite make sense.
I do have a more general concern however: this PR introduces new abstract functions/types in Arch_herd.S (for the constraint solver), but the abstraction feels fragile in both directions: implementations assume specific caller behavior (e.g., pp_solver_state must return "" for empty_solver because top_herd replaces solver states when debugging is disabled; similarly for compare_solver_state), and callers assume specific implementation details (e.g., top_herd relies on do_dump_final_state always including solver printing via pp_solver_state).
This creates a brittle contract that's not enforced by the types and makes reading the code harder (you can't understand local choices, like the "empty solver_state" trick at top_herd.ml:542, without knowing implementation details of distant call sites) and later refactors more prone to break things inadvertently.
My suggestion would be to either make those assumptions more explicit in the API, or keep the abstraction honest by removing caller‑specific behavior from implementations and implementation‑specific assumptions from call sites. I appreciate this is quite vague, so please refer to the in-code comments.
|
Thank you @HadrienRenaud for the review. I have now removed the Constraint indirection and emit predicates directly from Thank you @fsestini for the review. |
This PR continues #1235. It uses Predicates for handling PAC collisions. The PAC solver is rebuilt using the Predicates to preserve collision history across multiple
AUT*instructions.