Skip to content

Commit e523f93

Browse files
committed
C++: Fix inconsistent-loop-direction performance
This query seems to have been de-optimized by recent optimizer or stats changes. On libretro/libretro-uae, the query took 1 second on a warm cache with dist 89ad5f1 but took 9979 seconds with dist a3b9b6eb9. The slowness was due to a Cartesian product in `illDefined{Decr,Incr}ForStmt` between all the definitions and all the uses of `Variable v`. This would be no problem with the right join order, but that has apparently been lost. This commit factors out a pair of `pragma[noinline]` helper predicates to make sure the definitions (`v.getAnAssignedValue()`) and the uses (`v.getAnAccess()`) are queried and filtered in separate predicates. The performance problem can be seen in the tuple counts of this pipeline I interrupted during evaluation of `inconsistentLoopDirection::illDefinedDecrForStmt#ffff#shared`: 89716 ~3% {2} r1 = SCAN Variable::Variable::getAnAssignedValue_dispred#ff OUTPUT FIELDS {Variable::Variable::getAnAssignedValue_dispred#ff.<1>,Variable::Variable::getAnAssignedValue_dispred#ff.<0>} 89716 ~0% {3} r2 = JOIN r1 WITH DataFlowUtil::TExprNode#ff@staged_ext ON r1.<0>=DataFlowUtil::TExprNode#ff@staged_ext.<0> OUTPUT FIELDS {r1.<1>,DataFlowUtil::TExprNode#ff@staged_ext.<0>,DataFlowUtil::TExprNode#ff@staged_ext.<1>} 502539405 ~0% {4} r3 = JOIN r2 WITH Variable::Variable::getAnAccess_dispred#fb ON r2.<0>=Variable::Variable::getAnAccess_dispred#fb.<0> OUTPUT FIELDS {Variable::Variable::getAnAccess_dispred#fb.<1>,r2.<1>,r2.<2>,r2.<0>} return r3
1 parent 46d7792 commit e523f93

File tree

1 file changed

+28
-12
lines changed

1 file changed

+28
-12
lines changed

cpp/ql/src/Likely Bugs/Likely Typos/inconsistentLoopDirection.ql

Lines changed: 28 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,18 +23,26 @@ predicate candidateForStmt(ForStmt forStmt, Variable v, CrementOperation update,
2323
rel = forStmt.getCondition()
2424
}
2525

26-
predicate illDefinedDecrForStmt( ForStmt forstmt, Variable v, Expr initialCondition, Expr terminalCondition ) {
27-
exists(DecrementOperation dec, RelationalOperation rel |
28-
// decrementing for loop
29-
candidateForStmt(forstmt, v, dec, rel) and
26+
pragma[noinline]
27+
predicate candidateDecrForStmt(ForStmt forStmt, Variable v, VariableAccess lesserOperand, Expr terminalCondition) {
28+
exists(DecrementOperation update, RelationalOperation rel |
29+
candidateForStmt(forStmt, v, update, rel) and
3030

3131
// condition is `v < terminalCondition`
3232
terminalCondition = rel.getGreaterOperand() and
33-
v.getAnAccess() = rel.getLesserOperand() and
33+
lesserOperand = rel.getLesserOperand() and
34+
v.getAnAccess() = lesserOperand
35+
)
36+
}
37+
38+
predicate illDefinedDecrForStmt( ForStmt forstmt, Variable v, Expr initialCondition, Expr terminalCondition ) {
39+
exists(VariableAccess lesserOperand |
40+
// decrementing for loop
41+
candidateDecrForStmt(forstmt, v, lesserOperand, terminalCondition) and
3442

3543
// `initialCondition` is a value of `v` in the for loop
3644
v.getAnAssignedValue() = initialCondition and
37-
DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(rel.getLesserOperand())) and
45+
DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(lesserOperand)) and
3846

3947
// `initialCondition` < `terminalCondition`
4048
(
@@ -45,18 +53,26 @@ predicate illDefinedDecrForStmt( ForStmt forstmt, Variable v, Expr initialCondit
4553
)
4654
}
4755

48-
predicate illDefinedIncrForStmt( ForStmt forstmt, Variable v, Expr initialCondition, Expr terminalCondition ) {
49-
exists(IncrementOperation inc, RelationalOperation rel |
50-
// incrementing for loop
51-
candidateForStmt(forstmt, v, inc, rel) and
56+
pragma[noinline]
57+
predicate candidateIncrForStmt(ForStmt forStmt, Variable v, VariableAccess greaterOperand, Expr terminalCondition) {
58+
exists(IncrementOperation update, RelationalOperation rel |
59+
candidateForStmt(forStmt, v, update, rel) and
5260

5361
// condition is `v > terminalCondition`
5462
terminalCondition = rel.getLesserOperand() and
55-
v.getAnAccess() = rel.getGreaterOperand() and
63+
greaterOperand = rel.getGreaterOperand() and
64+
v.getAnAccess() = greaterOperand
65+
)
66+
}
67+
68+
predicate illDefinedIncrForStmt( ForStmt forstmt, Variable v, Expr initialCondition, Expr terminalCondition ) {
69+
exists(VariableAccess greaterOperand |
70+
// incrementing for loop
71+
candidateIncrForStmt(forstmt, v, greaterOperand, terminalCondition) and
5672

5773
// `initialCondition` is a value of `v` in the for loop
5874
v.getAnAssignedValue() = initialCondition and
59-
DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(rel.getGreaterOperand())) and
75+
DataFlow::localFlowStep(DataFlow::exprNode(initialCondition), DataFlow::exprNode(greaterOperand)) and
6076

6177
// `terminalCondition` < `initialCondition`
6278
(

0 commit comments

Comments
 (0)