Skip to content

Ssa: Trim the use-use relation to skip irrelevant nodes #19044

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.

Sign up for GitHub

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

Merged
aschackmull merged 15 commits into from
Mar 28, 2025
Merged

Ssa: Trim the use-use relation to skip irrelevant nodes #19044

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5aa7029
SSA: Add support for skipping WriteDefinitions in use-use.
aschackmull Mar 14, 2025
7c82f51
Java: Skip SSA definition nodes in data flow.
aschackmull Mar 14, 2025
c778bf6
SSA: Rename SsaInputDefinitionExt
aschackmull Mar 14, 2025
669f926
SSA: Skip irrelevant phi input nodes.
aschackmull Mar 14, 2025
4e2ad97
SSA: Skip phi nodes with unique successor.
aschackmull Mar 17, 2025
36532bc
SSA: Skip identity steps.
aschackmull Mar 17, 2025
0162b84
SSA: Fix a poor join-order and avoid SSA recomputation.
aschackmull Mar 18, 2025
b3bea97
C#: Accept test changes.
aschackmull Mar 18, 2025
f27e819
Java: Accept test changes.
aschackmull Mar 18, 2025
e7e5f75
Ruby: Accept test changes.
aschackmull Mar 18, 2025
ae47339
Rust: Accept test changes.
aschackmull Mar 18, 2025
4d04391
C++: Keep all phi input back edges.
aschackmull Mar 25, 2025
d5d0274
Java/SSA: Keep proper distinction between cached stages.
aschackmull Mar 25, 2025
8749bdb
C++: Accept test changes.
aschackmull Mar 25, 2025
c6cee48
SSA: Address review comments.
aschackmull Mar 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 35 additions & 2 deletions shared/ssa/codeql/ssa/Ssa.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,12 @@ module Make<LocationSig Location, InputSig<Location> Input> {
* definition.
*/
default predicate includeWriteDefsInFlowStep() { any() }

/**
* Holds if barrier guards should be supported on input edges to phi
* nodes. Disable this only if barrier guards are not going to be used.
*/
default predicate supportBarrierGuardsOnPhiEdges() { any() }
}

/**
Expand Down Expand Up @@ -1533,6 +1539,29 @@ module Make<LocationSig Location, InputSig<Location> Input> {
)
}

/**
* Holds if the input to `phi` from the block `input` might be relevant for
* barrier guards as a separately synthesized `TSsaInputNode`.
*/
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
Copy link
Contributor

@hvitved hvitved Mar 28, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@aschackmull aschackmull Mar 28, 2025

Choose a reason for hiding this comment

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

Yes.

(
exists(DfInput::Guard g | g.controlsBranchEdge(input, phi.getBasicBlock(), _))
or
exists(BasicBlock prev |
Copy link
Contributor

@hvitved hvitved Mar 28, 2025

Choose a reason for hiding this comment

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

I think this could deserve a comment with an example

AdjacentSsaRefs::adjacentRefPhi(prev, _, input, phi.getBasicBlock(),
phi.getSourceVariable()) and
prev != input and
exists(DfInput::Guard g, boolean branch |
DfInput::guardControlsBlock(g, input, branch) and
not DfInput::guardControlsBlock(g, prev, branch)
)
)
)
}

private newtype TNode =
TParamNode(DfInput::Parameter p) {
exists(WriteDefinition def | DfInput::ssaDefInitializesParam(def, p))
Expand All @@ -1546,7 +1575,7 @@ module Make<LocationSig Location, InputSig<Location> Input> {
)
} or
TSsaDefinitionNode(DefinitionExt def) or
TSsaInputNode(SsaPhiExt phi, BasicBlock input) { exists(getAPhiInputDef(phi, input)) }
TSsaInputNode(SsaPhiExt phi, BasicBlock input) { relevantPhiInputNode(phi, input) }

/**
* A data flow node that we need to reference in the value step relation.
Expand Down Expand Up @@ -1822,8 +1851,12 @@ module Make<LocationSig Location, InputSig<Location> Input> {
// Flow from definition/read to phi input
exists(BasicBlock input, BasicBlock bbPhi, DefinitionExt phi |
AdjacentSsaRefs::adjacentRefPhi(bb1, i1, input, bbPhi, v) and
nodeTo = TSsaInputNode(phi, input) and
phi.definesAt(v, bbPhi, -1, _)
|
nodeTo = TSsaInputNode(phi, input)
or
not relevantPhiInputNode(phi, input) and
nodeTo.(SsaDefinitionExtNodeImpl).getDefExt() = phi
)
}

Expand Down