Skip to content

C++: Fix global-variable flow for array types #14822

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 2 commits into from
Nov 20, 2023

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Nov 16, 2023

In #14736 we fixed a bunch of problems related to tracking indirections of global variables. One thing we forgot to handle was the subtle semantic difference between pointer types and array types. This PR fixes this by special-casing ArrayType similar to what we've been doing for SSA for a long time.

The first commit adds a lot of tests. I basically threw in any combination of direct/indirect and static local/global tests I could think of. The second PR fixes the tests that were failing by doing the above handlign of array types.

DCA looks uneventful. The first run showed no changes in performance/results. The second run (which should have had no performance-relevant differences compared to the first one) shows a significant speedup for a couple of projects that I can't explain. Luckily, it also shows no result changes.

@github-actions github-actions bot added the C++ label Nov 16, 2023
@MathiasVP MathiasVP force-pushed the fix-global-variable-flow-for-arrays branch from 22f25e8 to 8dc6c03 Compare November 17, 2023 16:05
@MathiasVP MathiasVP force-pushed the fix-global-variable-flow-for-arrays branch from 8dc6c03 to dcba8e5 Compare November 20, 2023 12:16
@MathiasVP MathiasVP marked this pull request as ready for review November 20, 2023 12:57
@MathiasVP MathiasVP requested a review from a team as a code owner November 20, 2023 12:57
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 20, 2023
Comment on lines +2 to +8
| test.cpp:864:44:864:58 | {...} | Node should have one enclosing callable but has 0. |
| test.cpp:864:47:864:54 | call to source | Node should have one enclosing callable but has 0. |
| test.cpp:872:46:872:51 | call to source | Node should have one enclosing callable but has 0. |
| test.cpp:872:53:872:56 | 1 | Node should have one enclosing callable but has 0. |
uniqueCallEnclosingCallable
| test.cpp:864:47:864:54 | call to source | Call should have one enclosing callable but has 0. |
| test.cpp:872:46:872:51 | call to source | Call should have one enclosing callable but has 0. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these suddenly showing up, while we don't have these for other globals for which we already had tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea. Since this is AST dataflow I didn't really bother to investigate this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is for AST dataflow, I missed that. Should have had a better look at the file names first. In that case, it seems safe to ignore.

Copy link
Contributor Author

@MathiasVP MathiasVP Nov 20, 2023

Choose a reason for hiding this comment

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

Note that line 864 is:

static const int global_array_dynamic[] = { ::source() };

and line 872 is:

static const int* global_pointer_dynamic = source(true);

So it's very possible that the code for these syntactic constructs simply was never generalized to handle these things appearing in global scope in AST dataflow (since AST dataflow never actually implementer proper support for global variables).

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

LGTM

@MathiasVP MathiasVP merged commit ab62606 into github:main Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants