-
Notifications
You must be signed in to change notification settings - Fork 73
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
Implement friendly front-end format #45
Conversation
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 looks good, not quite there — do you think we should land and iterate in tree? Or would you prefer to hack on the PR a bit more?
src/program.rs
Outdated
// the most common statement effects: mid point effects | ||
for effect in &statement.effects { | ||
match effect { | ||
// facts: region_live_at(Region, Point) |
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.
Hmm, so for region_live_at
, we have to do more — we need to actually compute liveness I think, but I realize that to do that we need to also have def('a)
, of course (and maybe we should make both def
and use
accept multiple lifetimes). We could also try to capture the idea of variables with types, though that is creeping towards MIR and I think I had hoped to do that as a second pass.
In other words, as an example:
bb0 {
def('a); // statement 0
; // statement 1
goto bb1;
}
bb1 {
; // statement 0
use('a); // statement 1
}
This ought to make 'a
live at all the following points:
- start(bb0/1),
- mid(bb0/1),
- start(bb1/0),
- mid(bb1/0),
- start(bb1/1),
- mid(bb1/1).
The idea would be: some variable X
whose type includes 'a
is used at mid(bb1/1). So, 'a
is live at that point, because the current value of X
matters -- this flows backwards until we reach mid(bb0/0)
, at which point X
was assigned, so its current value doesn't matter.
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.
We can of course use datafrog to do this computation =) you can imagine us emitting use
and def
facts and setting up a rule:
region_live_at(R, P) :- use(R, P).
region_live_at(R, P) :-
use(R, Q),
cfg_edge(P, Q),
!def(R, P).
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.
We also could potentially refactor the compiler to emit use
and def
facts and not region_live_at
facts.
src/program.rs
Outdated
facts.invalidates.insert((point, loan)); | ||
} | ||
|
||
_ => panic!("Unexpected effect: {:?}", effect), |
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 would rather factor Effect
into nested enums so that this case never happens.
Move AllFacts into polonius-engine Remove accidentally commited timely code, update datafrog
Implement a friendlier front-end format destined to help create isolated tests, without needing rustc NLL facts, but still generating the same data structure.
Alright I'll fix this and reopen this later tonight |
Implement the friendlier front-end format described in issue #26 — destined to help create isolated tests without needing rustc NLL facts, but still generating the same data structure.
Here's how it looks: