Skip to content

Conversation

jbj
Copy link
Contributor

@jbj jbj commented May 29, 2019

It turns out we didn't have to move the getName implementation into the mirror classes in QualifiedName. Doing so only made it harder for the optimiser to specialize calls to getName on various kinds of Declaration.

Most of the diff in this PR (the first commit) is a partial revert of #1303, reverting just the parts that cause unnecessary complication. The cost is that there are now references in both directions between QualifiedName.qll and Declaration.qll.

Specialization of getName still doesn't work as well as I would like, but this change does show some improvement. For example, current master has this DIL:

EscapesTree::stdIdentityFunction#f(pure unique Function::Function f) :-
  exists(pure Namespace::Namespace call_result |
    exists(pure Namespace::Namespace receiver |
      exists(pure Namespace::Namespace call_result#4 |
        exists(pure string arg1 | arg1 = "std", namespaces(call_result#4, arg1)),
        Declaration::Declaration::getNamespace#bf(f, call_result#4)
      ),
      (
        exists(string arg1 |
          arg1 = "move", Declaration::Declaration::getName_dispred#bb(f, arg1)
        );
        exists(string arg1 |
          arg1 = "forward",
          Declaration::Declaration::getName_dispred#bb(f, arg1)
        )
      ),
      Declaration::Declaration::getNamespace#bf(f, receiver),
      Namespace::Namespace::getParentNamespace_dispred#ff(receiver, call_result)
    ),
    exists(pure string arg1 | arg1 = "", namespaces(call_result, arg1))
  )
.

while this PR gets us this DIL:

EscapesTree::stdIdentityFunction#f(pure unique Function::Function f) :-
  exists(pure Namespace::Namespace call_result |
    exists(pure Namespace::Namespace receiver |
      exists(pure Namespace::Namespace call_result#4 |
        exists(pure string arg1 | arg1 = "std", namespaces(call_result#4, arg1)),
        Declaration::Declaration::getNamespace#bf(f, call_result#4)
      ),
      (
        exists(pure string arg1, pure dontcare int _ |
          arg1 = "move", functions(f, arg1, _)
        );
        exists(pure string arg1, pure dontcare int _ |
          arg1 = "forward", functions(f, arg1, _)
        )
      ),
      Declaration::Declaration::getNamespace#bf(f, receiver),
      Namespace::Namespace::getParentNamespace_dispred#ff(receiver, call_result)
    ),
    exists(pure string arg1 | arg1 = "", namespaces(call_result, arg1))
  )
.

The only difference is the disjunction that matches "move" and "forward".

jbj added 2 commits May 29, 2019 14:15
This reverts the `getName()` parts of 56e88cb and 0a2e288.
It turns out we didn't have to move the `getName` implementation into
the mirror classes in `QualifiedName`. Doing so only made it harder for
the optimiser to specialize calls to `getName` on various kinds of
`Declaration`.
@jbj jbj added the C++ label May 29, 2019
@jbj jbj requested a review from a team as a code owner May 29, 2019 12:37
@geoffw0
Copy link
Contributor

geoffw0 commented May 29, 2019

I've done a few performance checks and what I tried seems to be as quick, or slightly quicker with these changes. Difficult to tell what's going on in the 'Clause timing report' though.

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.

This was difficult to review because there's a fair bit of history explaining these changes (@jbj filled me in with some of the details on Slack). I won't pretend to be 100% happy but (1) most of this PR is reverting recent changes that were deemed to be a mistake (2) I've tested and it seems to behave well in practice and (3) there is good test coverage to ensure correctness.

@geoffw0 geoffw0 merged commit d672a6e into github:master May 30, 2019
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