Skip to content

C++: Fix IR variable reuse for global var inits #8912

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 15 commits into from
Jun 20, 2022

Conversation

rdmarsh2
Copy link
Contributor

Fixes a performance issue where all IRVariables corresponding to a given global variable were being used in the initializer IRFunction for that global variable.

@rdmarsh2 rdmarsh2 requested review from a team as code owners April 27, 2022 20:38
@rdmarsh2 rdmarsh2 added the no-change-note-required This PR does not need a change note label Apr 27, 2022
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

@jketema

This comment was marked as outdated.

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

dbartol
dbartol previously approved these changes Apr 27, 2022
Copy link

@dbartol dbartol left a comment

Choose a reason for hiding this comment

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

LGTM as long as the tests pass.

@jketema
Copy link
Contributor

jketema commented Apr 28, 2022

I fixed the test regressions caused by running against the old frontend. However, it turns out, also some syntax zoo tests broke (unrelated to the frontend). I could update those, but I cannot tell if the changes are correct.

Also the IR VariableAddresses without names look funny especially for declarations like:

int global_6 = global_2;

This becomes:

# 1769| int global_6
# 1769|   Block 0
# 1769|     v1769_1(void)       = EnterFunction     : 
# 1769|     mu1769_2(unknown)   = AliasedDefinition : 
# 1769|     r1769_3(glval<int>) = VariableAddress   : 
# 1769|     r1769_4(glval<int>) = VariableAddress   : 
# 1769|     r1769_5(int)        = Load[?]           : &:r1769_4, ~m?
# 1769|     mu1769_6(int)       = Store[?]          : &:r1769_3, r1769_5
# 1769|     v1769_7(void)       = ReturnVoid        : 
# 1769|     v1769_8(void)       = AliasedUse        : ~m?
# 1769|     v1769_9(void)       = ExitFunction      : 

Which VariableAddress is which here?

@MathiasVP MathiasVP mentioned this pull request Apr 28, 2022
@rdmarsh2 rdmarsh2 force-pushed the rdmarsh2/fix-ir-globals branch from 37fb227 to fe52dd9 Compare April 29, 2022 19:30
@rdmarsh2
Copy link
Contributor Author

rdmarsh2 commented May 2, 2022

Also the IR VariableAddresses without names look funny especially for declarations like:

Fixed by d1c6022

@jketema
Copy link
Contributor

jketema commented May 2, 2022

Fixed by d1c6022

Thanks. Would it be possible to add int global_6 = global_2; as a test?

@rdmarsh2
Copy link
Contributor Author

rdmarsh2 commented May 2, 2022

Good call - I've added the test and fixed another case of missing variables

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 9 vulnerabilities.

Comment on lines +949 to +951
/**
* Represents the IR translation of a root element, either a function or a global variable.
*/

Check warning

Code scanning / CodeQL

Class QLDoc style.

The QLDoc for a class should start with 'A', 'An', or 'The'.
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

Found 1 vulnerability.

result = this.getInstruction(InitializerVariableAddressTag())
or
tag = InitializerVariableAddressTag() and
result = getChild(1).getFirstInstruction()

Check warning

Code scanning / CodeQL

Using implicit `this`

Use of implicit `this`.
Copy link
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

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

Everything LGTM! Do we need to rerun DCA? I don't think much has changed apart from a merge-from-main and accepting some test changes, right?

@rdmarsh2
Copy link
Contributor Author

rdmarsh2 commented Jun 15, 2022

I don't think we do... My only worry would be how much change there's been to main in the meantime, but there hasn't been much else happening on the C++ dataflow side

@MathiasVP MathiasVP merged commit 35c8ca1 into github:main Jun 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants