Skip to content

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Feb 18, 2021

The predicate went from ~40s to ~100ms when running TaintedPath.ql on ace.

The goal of my reordering was mostly to get barrierGuardIsRelevant earlier in the join-order.

@erik-krogh erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Feb 18, 2021
@github-actions github-actions bot added the JS label Feb 18, 2021
@erik-krogh erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Feb 18, 2021
@erik-krogh erik-krogh marked this pull request as ready for review February 18, 2021 17:03
@erik-krogh erik-krogh requested a review from a team as a code owner February 18, 2021 17:03
@erik-krogh
Copy link
Contributor Author

Evaluation looks OK.

*
* This predicate exists to get a better join-order for the `barrierGuardBlocksEdge` predicate above.
*/
private BasicBlock getADominatedBasicBlock(BarrierGuardNode guard, ConditionGuardNode cond) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this perhaps be pragma[noinline] to prevent the compiler from inlining the predicate and discovering the bad join order again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe.
I prefer not to.
If the compiler is changed such that it inlines the predicate, it might also be changed such that it discovers a better join order.
If we use a pragma, we force the compiler to use the current join order, even if something better is possible in the future.

@asgerf
Copy link
Contributor

asgerf commented Feb 19, 2021

Can you hold off on this until after the Redux PR is merged, or run an evaluation of it based on that? It changes the dominates predicate in a significant way and I'd like to avoid a semantic merge conflict.

@asgerf
Copy link
Contributor

asgerf commented Feb 19, 2021

Can you hold off on this until after the Redux PR is merged

OK that was a bad suggestion given that it just hit the BDD node limit again. Nevermind.

@codeql-ci codeql-ci merged commit c5ae8d2 into github:main Feb 23, 2021
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.

4 participants