Skip to content
This repository was archived by the owner on Feb 3, 2020. It is now read-only.

BitfieldSimplifier: arbitrary precision bit masks - #11

Open
matrizzo wants to merge 2 commits into
S2E:masterfrom
matrizzo:improvement/ap-simplifier
Open

BitfieldSimplifier: arbitrary precision bit masks#11
matrizzo wants to merge 2 commits into
S2E:masterfrom
matrizzo:improvement/ap-simplifier

Conversation

@matrizzo

Copy link
Copy Markdown

Use llvm::APInt instead of uint64_t for bit masks.
Needed for the modified (non-forking) helper for signed multiplication
which uses 128-bit integers.

Signed-off-by: Matteo Rizzo matteo.rizzo@epfl.ch

Comment thread lib/Expr/BitfieldSimplifier.cpp Outdated

bits[0].ignoredBits = ignoredBits | bits[1].knownZeroBits;
bits[1].ignoredBits = ignoredBits | (bits[0].knownZeroBits & ~bits[1].knownZeroBits);
bits[1].ignoredBits = ignoredBits | bits[0].knownZeroBits;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the remaining part of the expression missing?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If a bit on the left side is 0, doesn't it mean that we can ignore the corresponding bit on the right side? And is also commutative so wouldn't it make sense to have the same expression for ignored bits on the left and on the right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could be, I'll have a better look later. In the meantime, could you move optimizations/bugfixes in a separate commit? The first commit would just have the mechanical conversion to APInt. This would make it much simpler to review.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In the original zeroMask is only needed because the masks are 64 bits wide even when the expressions are narrower than 64 bits. APInts can be any number of bits wide so that would not be needed anymore. Should I keep 64-bit masks and zeroMask in the mechanical conversion or not?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, please keep 64-bits.

@vitalych

Copy link
Copy Markdown
Member

I admit that this BitFieldSimplifier is a bit fuzzy in my mind. There are some expressions that look different from before, it would make sense to add comments to the code to explain the changes. Also a couple of test cases would help. They should be simple to add, following this model: https://github.com/S2E/klee/blob/master/unittests/Expr/ExprTest.cpp

@vitalych vitalych left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

.

Matteo Rizzo added 2 commits September 28, 2018 20:11
Still limited to 64-bit masks.

Signed-off-by: Matteo Rizzo <matteo.rizzo@epfl.ch>
Use llvm::APInt to support expressions of arbitrary width.

Signed-off-by: Matteo Rizzo <matteo.rizzo@epfl.ch>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants