Skip to content

Built foundation for activation literal management in PDR - #52

Open
michael-zhang-lambda wants to merge 14 commits into
cucapra:mainfrom
michael-zhang-lambda:feature/add-act-lit-scope-pool
Open

Built foundation for activation literal management in PDR#52
michael-zhang-lambda wants to merge 14 commits into
cucapra:mainfrom
michael-zhang-lambda:feature/add-act-lit-scope-pool

Conversation

@michael-zhang-lambda

Copy link
Copy Markdown
Contributor

This PR is essentially #50, but all activation literal usage that was not present before (including in PDR and BMC) are removed.

Thus, YICES2 will run all tests without (check-sat-assuming) and (get-unsat-assumptions).

Changes

  • Added new supports_check_assuming_exprs flag to mark solvers that can accept compound expression query assumptions
  • Removed create_act_lit from BasePdr
  • Implemented the ActLitScope struct that contains activation literals active in a given scope, which can then be permanently deactivated (i.e. assert $\neg \text{act}$ in solver) with the release method
  • Implemented the ActLitPool struct which provides helper methods to create activation literal implications (i.e. $\text{act} \Rightarrow \text{expr}$) in a ActLitScope, with an additional cache and special helper method (step_lit_act) to reuse activation literals for stepped cube literals (which are valid throughout the whole PDR run)
  • Added with_act_scope helper function to execute a closure with an activation literal scope, automatically disabling the relevant activation literals (i.e. those that are not associated with stepped cube literals)
  • Cleaned up stale documentation

Comment thread patronus/src/smt/solver.rs Outdated
fn name(&self) -> &str;
fn supports_check_assuming(&self) -> bool;
/// Indicates whether `(check-sat-assuming ...)` accepts arbitrary Boolean-valued expressions
fn supports_check_assuming_exprs(&self) -> bool {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't define this here. I would rather have each implementation of the trait provide their own explicit definition.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'll make the change.

@ekiwi ekiwi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is new code that is never used. It also looks like you removed some activation literal cleanups without replacement.

Comment thread patronus/src/mc/pdr.rs
}

/// Execute closure with a fresh [`ActLitScope`] and clean up all used activation literals in the end
fn with_act_scope<S: SolverContext, T>(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used?

@michael-zhang-lambda michael-zhang-lambda Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, we don't have any activation literals that we want to permanently disable yet. This will be necessary once we get activation literals for compound formulas that we would only use once for a query (e.g. compound expressions in a relative inductiveness query).

Currently, all activation literals are coupled with formulas that are used throughout the program (such as the FROM_STEP bad states, TO_STEP constraints, and various stepped cube literals). To throw these away would mean wasting more solver time to redefine them when we need them. Therefore, their activation literals are permanently asserted in the solver and are cached to be used again as needed.

I can remove this helper function in this PR if you want to keep things clean for this PR.

Comment thread patronus/src/mc/pdr.rs

/// Create a temporary activation literal that is coupled with `body` (i.e. `act => body`)
/// and registered into `scope`
fn imply(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the only activation literals that we create should persist throughout the entire PDR run. imply is only used when we want an activation literal to be active for some scope (connecting back the the reason why we have no usages for with_act_scope).

In a sense, we can say that imply is always coupled with a call to with_act_scope. Thus, it is not used in the current PDR implementation.

Comment thread patronus/src/mc/pdr.rs

if check_res.0 && first_iter {
// Clean up activation literals
for &act in lit_map.keys() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this cleanup happen now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the activation literals are associated with stepped literals (instead of compound formulas), they could be used in a different context. Therefore, to prevent redefinition of activation literals, step_lit_act will cache the activation literal for the particular stepped literal.

If we were to deactivate these activation literals here, we may get a solver error in the future since the cache will still return the cached activation literal (which was already deactivated).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if these literals are removed from the cube, that does not necessarily mean that the literal does not exist elsewhere. For example, let's say that some literal was truly dropped in the fix_gen_cube subroutine. However, the same literal could exist in a CTI in block_cube. Just because we stopped using the literal in one instance doesn't mean it's useless in another! In this case, it would better to reuse the same activation literal, instead of creating a new one.

Comment thread patronus/src/mc/pdr.rs

// Permanently disable literals that were removed
for &act in &prev_acts {
if !lit_map.contains_key(&act) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this cleanup happen now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as before.

Comment thread patronus/src/mc/pdr.rs
fin_lits.extend(lit_map.values().copied());

// Clean up activation literals
for &act in lit_map.keys() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as before.

Comment thread patronus/src/mc/pdr.rs
};

// Disable all created activation literals as cleanup
for &act in lit_map.keys() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as before.

Comment thread patronus/src/mc/pdr.rs Outdated
smt_ctx.restart()?;

// Clear the stepped literal cache
state.pool.step_lit_cache.clear();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you clear the cache here?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since you clear the cache, should you also disable the associated activation literals?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little bit of vestigial code from earlier. This code just wants to highlight that the cache for the pool is invalid (because the solver was previously cleared). Technically, this doesn't provide that much safety for the program because we still have stale solver formulas in the PdrEncodingWrapper, frame trace, etc. because the solver was restarted.

However, since PDR will subsequently call BMC and return, this is not an issue for now. I'm removing this to keep the code clean.

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

Successfully merging this pull request may close these issues.

2 participants