Skip to content

C++: Optimize SubBasicBlocks library #1824

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
pavgust merged 3 commits into from
Aug 26, 2019
Merged

C++: Optimize SubBasicBlocks library #1824

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
be24c64
C++: Optimize SubBasicBlock::getNumberOfNodes()
jbj Aug 26, 2019
d4f1cf9
C++: Optimize SubBasicBlock::getNode(int)
jbj Aug 26, 2019
5e67482
C++: Optimize SubBasicBlock::getPosInBasicBlock()
jbj Aug 26, 2019
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
42 changes: 29 additions & 13 deletions cpp/ql/src/semmle/code/cpp/dataflow/internal/SubBasicBlocks.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,18 @@ class SubBasicBlock extends ControlFlowNodeBase {
* `bb`, where `bb` is equal to `getBasicBlock()`.
*/
int getPosInBasicBlock(BasicBlock bb) {
exists(int nodePos, int rnk |
bb = this.(ControlFlowNode).getBasicBlock() and
this = bb.getNode(nodePos) and
nodePos = rank[rnk](int i | exists(SubBasicBlock n | n = bb.getNode(i))) and
exists(int thisIndexInBB, int rnk |
thisIndexInBB = this.getIndexInBasicBlock(bb) and
thisIndexInBB = rank[rnk](int i | i = any(SubBasicBlock n).getIndexInBasicBlock(bb)) and
result = rnk - 1
)
}

pragma[noinline]
private int getIndexInBasicBlock(BasicBlock bb) {
this = bb.getNode(result)
}

/** Gets a successor in the control-flow graph of `SubBasicBlock`s. */
SubBasicBlock getASuccessor() {
this.lastInBB() and
Expand All @@ -98,16 +102,22 @@ class SubBasicBlock extends ControlFlowNodeBase {
* start from 0, and the node at position 0 always exists and compares equal
* to `this`.
*/
pragma[nomagic]
ControlFlowNode getNode(int pos) {
exists(BasicBlock bb | bb = this.getBasicBlock() |
exists(int thisPos | this = bb.getNode(thisPos) |
result = bb.getNode(thisPos + pos) and
pos >= 0 and
pos < this.getNumberOfNodes()
exists(BasicBlock bb |
exists(int outerPos |
result = bb.getNode(outerPos) and
pos = outerPosToInnerPos(bb, outerPos)
)
)
}

pragma[nomagic]
private int outerPosToInnerPos(BasicBlock bb, int posInBB) {
posInBB = result + this.getIndexInBasicBlock(bb) and
result = [ 0 .. this.getNumberOfNodes() - 1 ]

@geoffw0 geoffw0 Aug 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How confident are we that this isn't quadratic? Naively, it seems that for any particular node bb, posInBB, every possible this in bb must be checked.

Other than this question, FWIW, these changes LGTM also.

@jbj jbj Aug 26, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the DIL we generate for this predicate:

noinline nomagic
SubBasicBlocks::SubBasicBlock::outerPosToInnerPos#ffff(/* SubBasicBlocks::SubBasicBlock */ cached entity this,
                                                       /* BasicBlocks::BasicBlock */ cached entity bb,
                                                       int posInBB, int result) :-
  exists(cached int call_result |
    exists(int range#high |
      exists(int Sub#lhs |
        SubBasicBlocks::SubBasicBlock::getIndexInBasicBlock#fff(this, bb,
                                                                call_result),
        SubBasicBlocks::SubBasicBlock::getNumberOfNodes#ff(this, Sub#lhs),
        Minus(Sub#lhs, 1, range#high)
      ),
      range(0, range#high, result)
    ),
    PlusInt(result, call_result, posInBB)
  )
.

It first joins in the two getters (functional relations), so the tuple count will be the number of SubBasicBlock. It then joins with the range, which multiplies that tuple count by the average number of nodes per SubBasicBlock. All together, this predicate should have the same number of tuples as there are ControlFlowNodes.

@geoffw0 geoffw0 Aug 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great, that gives me a bit more confidence.

}

/** Gets a control-flow node in this `SubBasicBlock`. */
ControlFlowNode getANode() {
result = this.getNode(_)
Expand Down Expand Up @@ -147,14 +157,20 @@ class SubBasicBlock extends ControlFlowNodeBase {
* Gets the number of control-flow nodes in this `SubBasicBlock`. There is
* always at least one.
*/
pragma[noopt]
int getNumberOfNodes() {
exists(BasicBlock bb | bb = this.getBasicBlock() |
exists(int thisPos | this = bb.getNode(thisPos) |
this.lastInBB() and
result = bb.length() - thisPos
exists(int bbLength |
this.lastInBB() and
bbLength = bb.length() and
result = bbLength - thisPos
)
or
exists(SubBasicBlock succ, int succPos |
succ.getPosInBasicBlock(bb) = this.getPosInBasicBlock(bb) + 1 and
exists(SubBasicBlock succ, int succPos, int thisRank, int succRank |
thisRank = this.getPosInBasicBlock(bb) and
succRank = thisRank + 1 and
succRank = succ.getPosInBasicBlock(bb) and
bb.getNode(succPos) = succ and
result = succPos - thisPos
)
Expand Down