Skip to content

C++ IR: guard against cycles in operand graph #1567

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.

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
merged 2 commits into from
Jul 11, 2019

Conversation

jbj
Copy link
Contributor

@jbj jbj commented Jul 9, 2019

This doesn't fix the underlying problem that for some reason there are cycles in the operand graph on our snapshots of the Linux kernel, but it ensures that the cycles don't lead to non-termination of ConstantAnalysis and ValueNumbering.

This doesn't fix the underlying problem that for some reason there are
cycles in the operand graph on our snapshots of the Linux kernel, but it
ensures that the cycles don't lead to non-termination of
`ConstantAnalysis` and `ValueNumbering`.
@jbj jbj added the C++ label Jul 9, 2019
@jbj jbj requested a review from dave-bartolomeo July 9, 2019 09:04
@jbj jbj requested a review from a team as a code owner July 9, 2019 09:04
@jbj
Copy link
Contributor Author

jbj commented Jul 9, 2019

I have to add another commit to improve performance. See https://semmle.slack.com/archives/C20TR4EAZ/p1562678869029300

@jbj jbj added the WIP This is a work-in-progress, do not merge yet! label Jul 9, 2019
Without this `pragma[noopt]`, `isInCycle` gets compiled into RA that
unpacks every tuple of the fast TC:

                      0          ~0%     {2} r1 = SELECT #Operand::getNonPhiOperandDef#3#ffPlus ON FIELDS #Operand::getNonPhiOperandDef#3#ffPlus.<0>=#Operand::getNonPhiOperandDef#3#ffPlus.<1>
                      0          ~0%     {1} r2 = SCAN r1 OUTPUT FIELDS {r1.<0>}
                                         return r2

With this change, it just becomes one lookup in the fast TC data
structure per instruction.
@jbj jbj removed the WIP This is a work-in-progress, do not merge yet! label Jul 9, 2019
@jbj
Copy link
Contributor Author

jbj commented Jul 9, 2019

I've added a second commit to improve performance of the cycle detection.

Copy link
Contributor

@dave-bartolomeo dave-bartolomeo left a comment

Choose a reason for hiding this comment

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

LGTM.
Please add a link (in a PR comment) to the Jira ticket for the underlying issue.

@jbj
Copy link
Contributor Author

jbj commented Jul 10, 2019

I've opened https://jira.semmle.com/browse/CPP-408 for the underlying issue.

@jbj
Copy link
Contributor Author

jbj commented Jul 11, 2019

The cycle detection is unfortunately slow on Wireshark, taking between 1 and 2 minutes in each of the #Operand::getNonPhiOperandDef#ffPlus predicates on my laptop.

I haven't been able to speed it up on Wireshark. There seems to be a chain of tens of thousand memory operands somewhere. I've tried limiting the cycle detection to register operands, but there are no such cycles on Linux. I've tried finding all the non-cyclic operands via recursion through forall, but that performs much worse.

The cycle detection is perfectly fast on other snapshots I've tried, taking only 2 seconds on Linux. And it's certainly better to have slow cycle detection on Wireshark than to have infinite loops on Linux.

@dave-bartolomeo dave-bartolomeo merged commit 1b38208 into github:master Jul 11, 2019
@jbj jbj mentioned this pull request Dec 10, 2019
3 tasks
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