Skip to content

Data flow: Call context virtual dispatch pruning in stage 1 #12124

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 4 commits into from
Feb 13, 2023

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Feb 7, 2023

Extracted from #11531.

When resolving dispatch targets in the first pruning stage, we may be able to restrict the number of viable targets by taking possible call contexts into account. That is, we don't actually track the precise call context, but instead we rely on all calls that might be a call context (in the same way that we handle flow through and field reads), using non-linear recursion.

Tuple counts before:

# n stage nodes fields conscand states tuples config
1 10 1 Fwd 4536712 27161 -1 1 4682396 RegExpConfiguration
2 15 1 Rev 1628246 21166 -1 1 1692253 RegExpConfiguration
3 20 2 Fwd 203065 3923 5698 1 383566 RegExpConfiguration
4 25 2 Rev 142916 3018 4331 1 169484 RegExpConfiguration
5 30 3 Fwd 85555 2221 99034 1 4963929 RegExpConfiguration
6 35 3 Rev 56430 1673 46080 1 1365534 RegExpConfiguration
7 40 4 Fwd 53035 1604 164343 1 12245345 RegExpConfiguration
8 45 4 Rev 50944 1577 146579 1 8293225 RegExpConfiguration
9 50 5 Fwd 44258 1129 6327 1 399404 RegExpConfiguration
10 55 5 Rev 14153 313 1597 1 134533 RegExpConfiguration
11 60 6 Fwd 13443 311 778 1 49836 RegExpConfiguration
12 65 6 Rev 433 13 15 1 442 RegExpConfiguration

Tuple counts after:

# n stage nodes fields conscand states tuples config
1 10 1 Fwd 4340437 26616 -1 1 4465642 RegExpConfiguration
2 15 1 Rev 1557411 20603 -1 1 1612848 RegExpConfiguration
3 20 2 Fwd 189527 3859 5575 1 362318 RegExpConfiguration
4 25 2 Rev 140906 3006 4310 1 167080 RegExpConfiguration
5 30 3 Fwd 85249 2223 99036 1 4905484 RegExpConfiguration
6 35 3 Rev 56070 1674 45877 1 1346324 RegExpConfiguration
7 40 4 Fwd 52628 1605 164780 1 12166793 RegExpConfiguration
8 45 4 Rev 50520 1578 147407 1 8212395 RegExpConfiguration
9 50 5 Fwd 44120 1130 6327 1 399064 RegExpConfiguration
10 55 5 Rev 14153 313 1597 1 134536 RegExpConfiguration
11 60 6 Fwd 13443 311 778 1 49836 RegExpConfiguration
12 65 6 Rev 433 13 15 1 442 RegExpConfiguration

@hvitved hvitved added the no-change-note-required This PR does not need a change note label Feb 7, 2023
@hvitved hvitved marked this pull request as ready for review February 8, 2023 13:10
@hvitved hvitved requested review from a team as code owners February 8, 2023 13:10
) {
exists(DataFlowCall ctx |
fwdFlowIsEntered(ctx, _, config) and
result = viableImplInCallContextExt(call, ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to say that something looked wrong here, but that was because I was thinking about prunedViableImplInCallContext rather than viableImplInCallContextExt. Could you add a flow test that fails if prunedViableImplInCallContext was mistakenly used here instead of viableImplInCallContextExt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

(
cc = false
or
not reducedViableImplInCallContext(call, _, _)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need cc = true and here in order to not get tuple duplication in this pipeline split.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't hurt.

fwdFlowOutFromArg(call, node, config) and
fwdFlowIsEntered(call, cc, config)
)
}

// inline to reduce the number of iterations
pragma[inline]
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you compare performance of this inlining against the nomagic alternative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't remember, but it probably doesn't matter that much. The logic was previously manually inlined into fwdFlow and fwdFlowIsEntered, so I decided to keep it that way.

@aschackmull aschackmull merged commit e877b16 into github:main Feb 13, 2023
@hvitved hvitved deleted the dataflow/stage1-dispatch branch February 13, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants