-
Notifications
You must be signed in to change notification settings - Fork 0
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
IR Changes #188
IR Changes #188
Changes from 11 commits
b5ae130
2354dbe
4da2ea4
143b330
fbbc930
199a745
d5dc2f2
e89a976
523b0d4
d8e528b
fa071bb
78a0bf6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,9 +19,9 @@ trait ANRAnalysis(cfg: ProgramCfg) { | |
|
||
val first: Set[CfgNode] = Set(cfg.startNode) | ||
|
||
private val stackPointer = Register("R31", BitVecType(64)) | ||
private val linkRegister = Register("R30", BitVecType(64)) | ||
private val framePointer = Register("R29", BitVecType(64)) | ||
private val stackPointer = Register("R31", 64) | ||
private val linkRegister = Register("R30", 64) | ||
private val framePointer = Register("R29", 64) | ||
|
||
private val ignoreRegions: Set[Expr] = Set(linkRegister, framePointer, stackPointer) | ||
|
||
|
@@ -35,12 +35,12 @@ trait ANRAnalysis(cfg: ProgramCfg) { | |
case assert: Assert => | ||
m.diff(assert.body.variables) | ||
case memoryAssign: MemoryAssign => | ||
m.diff(memoryAssign.lhs.variables ++ memoryAssign.rhs.variables) | ||
m.diff(memoryAssign.index.variables) | ||
case indirectCall: IndirectCall => | ||
m - indirectCall.target | ||
case localAssign: LocalAssign => | ||
m = m.diff(localAssign.rhs.variables) | ||
if ignoreRegions.contains(localAssign.lhs) then m else m + localAssign.lhs | ||
case assign: Assign => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still in favour of having a separate load instruction so that assign is clearly pure computation, but its not essential. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we've discussed before. this means creating a new temporary variable for nearly every load, adding a lot of bloat to the IR without actually improving anything. |
||
m = m.diff(assign.rhs.variables) | ||
if ignoreRegions.contains(assign.lhs) then m else m + assign.lhs | ||
case _ => | ||
m | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is worth it, it might cause problems if we want to change back to integers e.g. if we are interested in something like the SMACK int selection with cegar paper. Trying to make register an alias for global bitvector variable using inheritance is a bit too tricky imo. It seems cleaner to me to only have regular variables where scope is a parameter/field rather than a trait mixin. We could approach variables being shared similarly, but I do think it makes sense to have shared and unshared be type-incompatible. E.g.
(as implemented as a trait/case class heirachy this just means moving name,type,scope,shared to the parent trait's constructor?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR doesn't make any changes to the scope. If you want to propose changes to how the scope works, you can do that separately. What you're suggesting would make the type system weaker though. What this PR does change is make it so Memory is not an Expr, and so (correctly) is no longer allowed to appear arbitrarily inside other Exprs.
I did consider the implications for supporting an integer representation (as in SMACK) but that would require wider changes anyway, as everything else already assumes everything is bitvectors - the Memory case class already does for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, if we want to lift registers to variables then we can lift registers to variables.