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

Add support for indexing register array #81

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ffengyu
Copy link

@ffengyu ffengyu commented Jul 22, 2024

Isla doesn't support pmpcfg and pmpaddr in the constraints because their type is
both ir::Val::Vector<_>, which falls into the default failure match arm of
function smt_value in file primop_until.rs.

There are two ways to fix it: extract the element from the vector before passing
it to smt_value, or provide the new matching pattern for vector in
smt_value. Considering Z3's array theory is for an infinite array, we must add
many constraints to imitate the extensional behavior of fixed-size array. In the
meantime, seq theory is not supported in Isla at all. We choose the first
option.

We extract the option in the get_loc_and_initialize because the arguments of
smt_value are got from the result of this function. We add additional Loc type
in smt_parser.lalrpop to combine the index and array together.

In the linearization.rc file, the ssa number is always -1 for the new Loc type because
the index is not variable and not assignable.

Copy link
Contributor

@bacam bacam left a comment

Choose a reason for hiding this comment

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

It would be nice to have a test too. I've just written one; I'll see if I can push it to your branch...

@@ -157,13 +157,15 @@ pub enum BlockLoc {
// Field locations contain the previous name so that we can update one field at a time
Field(Box<BlockLoc>, SSAName, SSAName),
Addr(Box<BlockLoc>),
// Index is not assignable and don't need SSA Name, so the ssa number is always -1.
Index(Box<BlockLoc>, SSAName),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this just use a u32 like Loc?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's essentially a wrapper of u32. I use SSAName for consistency in style.

Copy link
Collaborator

@Alasdair Alasdair Jul 22, 2024

Choose a reason for hiding this comment

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

It should just be u32. It should only be a SSAName if it was originally a Name in the non-SSA IR.

Name::from_u32(n) could maybe have a more descriptive name - it is not just creating a Name for the number, it's creating a name that indexes the nth element of the symbol table which is not what we want here.

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.

3 participants