Skip to content

C++: Don't use GVN in AST DataFlow BarrierNode#2755

Merged
MathiasVP merged 1 commit intogithub:masterfrom
jbj:BarrierGuard-SSA
Feb 4, 2020
Merged

C++: Don't use GVN in AST DataFlow BarrierNode#2755
MathiasVP merged 1 commit intogithub:masterfrom
jbj:BarrierGuard-SSA

Conversation

@jbj
Copy link
Contributor

@jbj jbj commented Feb 4, 2020

It turns out that the evaluator will evaluate the GVN stage even when no predicate from it is needed after optimization of the subsequent stages. The GVN library is expensive to evaluate, and it'll become even more expensive when we switch its implementation to IR.

This PR disables the use of GVN in DataFlow::BarrierNode for the AST data-flow library, which should improve performance when evaluating a single data-flow query on a snapshot with no cache. Precision decreases slightly, leading to a new FP in the qltests.

There is no corresponding change for the IR data-flow library since IR GVN is not very expensive.

It turns out that the evaluator will evaluate the GVN stage even when no
predicate from it is needed after optimization of the subsequent stages.
The GVN library is expensive to evaluate, and it'll become even more
expensive when we switch its implementation to IR.

This PR disables the use of GVN in `DataFlow::BarrierNode` for the AST
data-flow library, which should improve performance when evaluating a
single data-flow query on a snapshot with no cache. Precision decreases
slightly, leading to a new FP in the qltests.

There is no corresponding change for the IR data-flow library since IR
GVN is not very expensive.
@jbj jbj added the C++ label Feb 4, 2020
@jbj jbj requested a review from geoffw0 February 4, 2020 07:54
@jbj jbj requested a review from a team as a code owner February 4, 2020 07:54
@jbj
Copy link
Contributor Author

jbj commented Feb 4, 2020

This is the change I talked about in the team meeting yesterday. The BarrierGuard feature was introduced in #2213.

Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

LGTM!

@MathiasVP MathiasVP merged commit 0276c97 into github:master Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants