Skip to content

C++/C#: Cleanup in preparation for indirect alias analysis #2421

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 10 commits into from
Nov 26, 2019

Conversation

dbartol
Copy link
Contributor

@dbartol dbartol commented Nov 22, 2019

This PR contains the first several commits from #1736, appropriately fixed up to handle the introduction of the IR type system and the sharing of the IR code with C#. These are all just cleanup and refactoring changes that came out of the indirect alias analysis work. I figured I'd get them in now, to avoid having them bit rot while I finish getting the rest of that PR in shape.

@dbartol dbartol requested review from a team as code owners November 22, 2019 20:25
Copy link
Contributor

@jbj jbj left a comment

Choose a reason for hiding this comment

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

Is performance still okay after the IRVariable changes?

@@ -0,0 +1,2 @@
private import SSAConstruction as SSA
import SSA::SSASanity
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this file? Can't we just write import SSAConstruction::SSASanity in SSASanity.ql?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we could get rid of it, but it does fit the pattern we've used for IRSanity.qll, where each sanity query .ql file is just some metadata (which has to be specialized for the flavor of the IR) plus a single import.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. We can come back to this question as part of a later reform of the sanity code (moving it out of Instruction.qll).

@dbartol
Copy link
Contributor Author

dbartol commented Nov 25, 2019

On ChakraCore, I measured a performance speedup of 5% with these changes, which is probably within the measurement error for running perf numbers on my laptop.

@jbj
Copy link
Contributor

jbj commented Nov 26, 2019

Tests are failing.

@jbj jbj merged commit c05cc77 into github:master Nov 26, 2019
@dbartol dbartol deleted the dbartol/IndirectAlias branch November 26, 2019 21:05
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.

3 participants