Skip to content

Conversation

@jbj
Copy link
Contributor

@jbj jbj commented Nov 5, 2018

As noted by @geoffw0 in #401, the previous formulation of this predicate caused a CP in snapshots where a variable had a large number of definitions and also reached a large number of sub-basic-blocks.

This should fix performance of https://github.com/FrodeSolheim/fs-uae and https://github.com/libretro/libretro-uae.

The FlowVar.getAnAccess predicate is still at risk of CP'ing when a large group of defs has a large group of uses, but that has not been observed to happen in practice yet. We would need to make localFlowStep expose phi definitions in order to avoid that risk.

The previous formulation of this predicate caused a CP in snapshots
where a variable had a large number of definitions and also reached a
large number of sub-basic-blocks.

This should fix performance of https://github.com/FrodeSolheim/fs-uae
and https://github.com/libretro/libretro-uae.

The `FlowVar.getAnAccess` predicate is still at risk of CP'ing when a
large group of defs has a large group of uses, but that has not been
observed to happen in practice yet. We would need to make
`localFlowStep` expose phi definitions in order to avoid that risk.
@jbj jbj added the C++ label Nov 5, 2018
@jbj jbj requested a review from geoffw0 November 5, 2018 10:07
@jbj jbj requested a review from a team as a code owner November 5, 2018 10:07
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

My runs of libretro-uae improve from 1049s to 745s with this change (compared to 1337s a few days ago). I'm happy and will merge this assuming the tests have passed.

If we're looking for further improvements there's still one expensive predicate left of the three I originally mentioned on Slack:

inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff .. 6m30s

@geoffw0 geoffw0 merged commit 5cd7103 into github:master Nov 5, 2018
@jbj
Copy link
Contributor Author

jbj commented Nov 5, 2018

@geoffw0 I can't replicate that performance problem. On libretro-uae I get the following, using the latest master from this repo in QL4E 1.18.0.

	inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff ....... 455ms

@geoffw0
Copy link
Contributor

geoffw0 commented Nov 6, 2018

That's great, we should keep an eye on the query to confirm though.

@geoffw0
Copy link
Contributor

geoffw0 commented Nov 30, 2018

For the rc/1.19 branch on my laptop, inconsistentLoopDirection.ql takes 789s on libretro-uae. This PR seems to be working fine but AlwaysTrueUponEntryLoop::getARelevantVariable is still a problem for me:

	inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff .. 6m36s
	inconsistentLoopDirection.ql-6:ControlFlowGraph::Cached::reachable#f ................................................. 22.1s (executed 19458 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getUpperBoundsImpl#ff ........................................... 12.7s (executed 398 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getLowerBoundsImpl#ff ........................................... 12.2s (executed 347 times)

On ql-master it similarly took 776s with:

	inconsistentLoopDirection.ql-11:FlowVar::FlowVar_internal::AlwaysTrueUponEntryLoop::getARelevantVariable_dispred#ff .. 6m35s
	inconsistentLoopDirection.ql-6:ControlFlowGraph::Cached::reachable#f ................................................. 22s (executed 19458 times)
	inconsistentLoopDirection.ql-6:ConstantExprs::potentiallyReturningFunction#f ......................................... 12.1s (executed 19457 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getUpperBoundsImpl#ff ........................................... 11.8s (executed 398 times)

@jbj
Copy link
Contributor Author

jbj commented Nov 30, 2018

Good catch. I'll take a look.

@jbj
Copy link
Contributor Author

jbj commented Dec 3, 2018

I should have investigated that problem better when you reported it the first time. The predicate had the shape exists(...) and exists(...), which is always dangerous for the optimiser. I've applied the standard fix in #598.

@geoffw0
Copy link
Contributor

geoffw0 commented Dec 3, 2018

With #598 it took just 370s and there are no standout predicates:

	inconsistentLoopDirection.ql-6:ControlFlowGraph::Cached::reachable#f ................................................. 21.5s (executed 19458 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getUpperBoundsImpl#ff ........................................... 12.4s (executed 398 times)
	inconsistentLoopDirection.ql-6:ConstantExprs::potentiallyReturningFunction#f ......................................... 12.2s (executed 19457 times)
	inconsistentLoopDirection.ql-10:SimpleRangeAnalysis::getLowerBoundsImpl#ff ........................................... 11.7s (executed 347 times)

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