-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Data flow: Compute local big step relation as stage output #16970
Conversation
7db654a
to
ae76a2e
Compare
348da87
to
88f4e98
Compare
34140b3
to
6ef2cdd
Compare
6ef2cdd
to
858f3e8
Compare
CPP DCA LGTM; two results lost, but I've seen these two wobble a few times lately. 👍 Swift DCA LGTM; just some extractor wobble. Analysis time change likely to be wobble as well, I can't rule out a real slowdown, but I trust you've done more accurate performance checks elsewhere. 👍 |
858f3e8
to
6eaba7c
Compare
@@ -1394,13 +1380,13 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> { | |||
CcNoCall getCallContextReturn(DataFlowCallable c, DataFlowCall call); | |||
|
|||
bindingset[cc] | |||
LocalCc getLocalCc(Cc cc); | |||
LocalCallContext getLocalCc(Cc cc); |
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.
What's going on here? To no longer parameterise over LocalCc
in the stages seems like a non-trivial change that's unrelated to the PR description. Also, getLocalCc
is expected to be functional, which now no longer holds.
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 have reverted this change.
@@ -3709,16 +3725,9 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> { | |||
} | |||
|
|||
import CallContextSensitivity<CallContextSensitivityInput> | |||
import NoLocalCallContext | |||
import LocalCallContext |
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.
A change like this would be nice to perf-check on its own.
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 have reverted this change.
@@ -3800,16 +3809,7 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> { | |||
|
|||
import BooleanCallContext |
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 here is why it's problematic to remove the parameterisation on LocalCc
. It's possible that we want to always use call-context sensitivity + local call context sensitivity, but if so, then that should be investigated on its own.
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 have reverted this change.
Is there a motivating example where this results in a significant reduction? Otherwise, my gut inclination is to not do this. What's the DIL size cost? |
6eaba7c
to
e5d626f
Compare
I have updated this PR so we now only recompute the big-step relation in stage 6 (using the big step relation from stage 3 as input). |
exists(NodeEx node0, boolean preservesValue0, DataFlowType t0, string label0, Ap ap | | ||
Input::localStep(node0, state, node2, state, preservesValue0, t0, cc, label0) and | ||
revFlow(node2, pragma[only_bind_into](state), pragma[only_bind_into](ap)) and | ||
not outBarrier(node0, state) and | ||
(preservesValue = true or ap instanceof ApNil) | ||
| | ||
node1 = node0 and | ||
localFlowEntry(node1, pragma[only_bind_into](state), pragma[only_bind_into](ap)) and | ||
preservesValue = preservesValue0 and | ||
label = label0 and | ||
t = t0 and | ||
node1 != node2 | ||
or | ||
exists(boolean preservesValue1, DataFlowType t1, string label1 | | ||
localFlowStepPlus(node1, pragma[only_bind_into](state), node0, preservesValue1, t1, | ||
cc, label1) and | ||
not node0 instanceof FlowCheckNode and | ||
preservesValue = preservesValue0.booleanAnd(preservesValue1) and | ||
label = mergeLabels(label1, label0) and | ||
if preservesValue0 = true then t = t1 else t = t0 |
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.
A few variable renamings that I think makes the code a bit clearer.
exists(NodeEx node0, boolean preservesValue0, DataFlowType t0, string label0, Ap ap | | |
Input::localStep(node0, state, node2, state, preservesValue0, t0, cc, label0) and | |
revFlow(node2, pragma[only_bind_into](state), pragma[only_bind_into](ap)) and | |
not outBarrier(node0, state) and | |
(preservesValue = true or ap instanceof ApNil) | |
| | |
node1 = node0 and | |
localFlowEntry(node1, pragma[only_bind_into](state), pragma[only_bind_into](ap)) and | |
preservesValue = preservesValue0 and | |
label = label0 and | |
t = t0 and | |
node1 != node2 | |
or | |
exists(boolean preservesValue1, DataFlowType t1, string label1 | | |
localFlowStepPlus(node1, pragma[only_bind_into](state), node0, preservesValue1, t1, | |
cc, label1) and | |
not node0 instanceof FlowCheckNode and | |
preservesValue = preservesValue0.booleanAnd(preservesValue1) and | |
label = mergeLabels(label1, label0) and | |
if preservesValue0 = true then t = t1 else t = t0 | |
exists(NodeEx mid, boolean preservesValue2, DataFlowType t2, string label2, Ap ap | | |
Input::localStep(mid, state, node2, state, preservesValue2, t2, cc, label2) and | |
revFlow(node2, pragma[only_bind_into](state), pragma[only_bind_into](ap)) and | |
not outBarrier(mid, state) and | |
(preservesValue = true or ap instanceof ApNil) | |
| | |
node1 = mid and | |
localFlowEntry(node1, pragma[only_bind_into](state), pragma[only_bind_into](ap)) and | |
preservesValue = preservesValue2 and | |
label = label2 and | |
t = t2 and | |
node1 != node2 | |
or | |
exists(boolean preservesValue1, DataFlowType t1, string label1 | | |
localFlowStepPlus(node1, pragma[only_bind_into](state), mid, preservesValue1, t1, | |
cc, label1) and | |
not mid instanceof FlowCheckNode and | |
preservesValue = preservesValue2.booleanAnd(preservesValue1) and | |
label = mergeLabels(label1, label2) and | |
if preservesValue2 = true then t = t1 else t = t2 |
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.
One minor comment, otherwise LGTM.
preservesValue, t, lcc, label) | ||
} | ||
|
||
bindingset[node1, state1] |
Check warning
Code scanning / CodeQL
Candidate predicate not marked as `nomagic` Warning
reducedViableImplInCallContext
Candidate predicate to
This PR moves the
LocalFlowBigStep
module from between stage 2 and stage 3 into the stage module, calculated as an output of the stage. TheLocalFlowBigStep
module is parameterized by an existing step relation, and we invoke the module inStage3Param
(as before this PR), and inStage6Param
, using the big step relation fromStage3Param
as input.The benefit of doing this is that we can sometimes skip intermediate nodes, resulting in smaller
edges
relations (as seen in the changes to the expected test output).