Ssa: Trim the use-use relation to skip irrelevant nodes #19044
Conversation
aschackmull
added
the
no-change-note-required
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (2)
- java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll: Language not supported
- shared/ssa/codeql/ssa/Ssa.qll: Language not supported
Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more
|
Looking at the DCA runs:
I'm a bit surprised because from the PR description I wasn't expecting to see changes in results. |
|
I've restarted dca after fixing a performance bug, the 3.8% slowdown on Rust is now 0.1% instead. As for the data flow inconsistencies (for e.g. Rust), these were fixed by the commit "SSA: Skip identity steps". There should be no need to run dca for Swift, since Swift doesn't use the Data Flow Integration module. As I mentioned on slack, result changes should not occur, but they do for C++ due to the somewhat ad-hoc |
|
Hmm, looks like at least Java needs some tweaking before this can work as intended. The dependence of the trimming on |
aschackmull
force-pushed
the
ssa/useuse-trim
branch
from
f797ede to
d5d0274
Compare
|
I've fixed the caching aspect for Java, and introduced a hook for C++, such that the |
| @@ -1495,6 +1495,13 @@ module Make<LocationSig Location, InputSig<Location> Input> { | |||
|
|
|||
| /** Holds if `guard` controls block `bb` upon evaluating to `branch`. */ | |||
| predicate guardControlsBlock(Guard guard, BasicBlock bb, boolean branch); | |||
|
|
|||
| /** | |||
| * Holds if `WriteDefinition`s should be included as an intermediate node | |||
There was a problem hiding this comment.
This is restricted to certain WriteDefinitions, right?
There was a problem hiding this comment.
Yes and no. Flipping this switch will remove all certain writes from the use-use steps. It will also cause us to step over uncertain write nodes for the RHS-to-first-use steps, but those nodes will remain in the graph as stepping stones from prior uses. It's potentially possible to skip over uncertain writes when stepping from prior uses, but that requires some additional analysis to ensure that we only do so when it won't cause blowups, and the potential gain seemed so small that I didn't bother with that particular graph reduction.
| private predicate relevantPhiInputNode(SsaPhiExt phi, BasicBlock input) { | ||
| DfInput::supportBarrierGuardsOnPhiEdges() and | ||
| // If the input isn't explicitly read then a guard cannot check it. | ||
| exists(DfInput::getARead(getAPhiInputDef(phi, input))) and |
There was a problem hiding this comment.
So this means that for
if (b) {
x = foo;
read(x);
}
else {
x = bar;
}
read(x);we will not generate an input edge from the else block?
| ( | ||
| exists(DfInput::Guard g | g.controlsBranchEdge(input, phi.getBasicBlock(), _)) | ||
| or | ||
| exists(BasicBlock prev | |
There was a problem hiding this comment.
I think this could deserve a comment with an example
shared/ssa/codeql/ssa/Ssa.qll
Outdated
| if phiHasUniqNextNode(phi) | ||
| then flowFromRefToNode(v, bbPhi, -1, nodeTo) | ||
| else nodeTo.(SsaDefinitionExtNodeImpl).getDefExt() = phi |
There was a problem hiding this comment.
Pull out into separate predicate to avoid code duplication?
This PR contains 3 tweaks to the shared SSA use-use step relation in the data flow integration module. Each of them trims the step relation in order to generate fewer nodes and fewer edges.
WriteDefinitions are skipped for Java. There should be no need to include an extra node in the step from the RHS of an assignment to the first use of the variable, but some languages may depend on the flow out of definitions for now, so so far this is opt-in only.