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

[HWToSMT] ArrayCreateOp and ArrayGetOp support #7666

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maerhart
Copy link
Member

@maerhart maerhart commented Oct 4, 2024

Resolves #7664

@uenoku
Copy link
Member

uenoku commented Oct 4, 2024

Thank you for quickly working on adding this! Using array theory sounds interesting but IIRC array theory is fairly expensive compared to bitvector. I think there are several other representations and I'm curious if ordinary dataflow representation might make sense.

  1. Lower to smt::ArrayStore/ArraySelect
  2. Lower it as comb.concat + comb.shru + comb.extract.
  3. Lower it as mux chains.

I created larger test case for (1) and (2) representation (https://gist.github.com/uenoku/7fcf89f676ff3d586204669c044ea924) and ran LEC against the module itself. LEC didn't finish for (1) within 2 minutes whereas it finished less than a second for (2).

@maerhart
Copy link
Member Author

maerhart commented Oct 4, 2024

Thanks for this very cool performance analysis! There are definitely a lot of representations we can try and benchmark against each other. Staying within BV theory also has the advantage that we can use SMT solvers optimized for BV only and don't support array theory.

There are two things about your concat+shru+extract version I'd like to point out:

  • i1 element types are the most convenient for this representation. For wider integers we'd additionally need a bitvector multiplication. Those are very problematic, but the usual optimizations can be done here, such as replacing it with a shift or concat if the integer width is a power of 2. Alternatively, we can pad the array elements to the a power of 2 when concatenating them to the big BV, which allows us to always use a concat instead of a multiplication.
  • Out-of-bounds accesses are not handled correctly. An OOB access is just defined to be zero here, but we'd need to return a symbolic value (I'm not sure we properly defined this, I'm just assuming we use the same semantics as division by 0)

Furthermore, we should be careful with benchmarking the approaches by just comparing the same circuit with itself.
I changed the array lowering to use your BV approach and incorporated above two points (not optimizing away the SMT multiplication and using i128 as array element types). Proving it equivalent with itself also takes ~1 sec, but proving it equivalent with the Comb implementation using a concat instead of the multiplication didn't terminate within 10 min.

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

i1 element types are the most convenient for this representation. For wider integers we'd additionally need a bitvector multiplication.

Yeah, that's a great point. I agree that using array theory is good for non-i1 integer arrays.

Out-of-bounds accesses are not handled correctly.

Yeah but can't we insert smt.ite for this representation as well while lowering hw.array_get?

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

I think starting from simple solution is always great so lowering it to ArrayStore and ArraySelect look good to me! Maybe we can do something complicated in the future when array-theory does not scale well 😃

Copy link
Contributor

@TaoBi22 TaoBi22 left a comment

Choose a reason for hiding this comment

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

LGTM! Very cool performance analysis @uenoku, agreed that it's worth getting this in for now and potentially moving to something more performant later!

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.

[HWToSMT] Support hw.array_create and hw.array_get
3 participants