Skip to content

C++: IR generation for repeated initializers #12755

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 8 commits into from
Apr 4, 2023

Conversation

MathiasVP
Copy link
Contributor

@MathiasVP MathiasVP commented Apr 4, 2023

This implements the IR changes necessary after #12712 to support repeated initializers.

Commit-by-commit review recommended.

  • The first commit adds @sashabu's failing tests
  • The second commit implements proper IR support
  • The third commit accepts the test changes (all of which are good)
  • The final commit renames a couple of the library predicates to properly reflect that they can have multiple results.

@MathiasVP MathiasVP requested a review from a team as a code owner April 4, 2023 09:18
@github-actions github-actions bot added the C++ label Apr 4, 2023
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Apr 4, 2023
@MathiasVP
Copy link
Contributor Author

DCA looks fine: Performance looks unchanged, no new results, and some IR inconsistencies have been fixed 🎉

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.

Mostly looks sensible. Few small remarks. Also a question. Consider:

struct Foo {
 int x[2];
 int y;
};

int main() {
  struct Foo f = { .x = {0, 1}, .x[0] = 42, .y = 42 };
  return 0;
}

What happens in this case?

* This predicate may have multiple results since a field can be initialized
* multiple times in the same initializer.
*/
Expr getFieldExpr(Field field) { result = this.getFieldExpr(field, _) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Expr getFieldExpr(Field field) { result = this.getFieldExpr(field, _) }
deprecated Expr getFieldExpr(Field field) { result = this.getFieldExpr(field, _) }

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 was planning on doing this as a follow-up since these predicates are used a bunch of places in. So we'd need to replace those when we formally deprecate these.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me.

* This predicate may have multiple results since an element can be initialized
* multiple times in the same initializer.
*/
Expr getElementExpr(int elementIndex) { result = this.getElementExpr(elementIndex, _) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Expr getElementExpr(int elementIndex) { result = this.getElementExpr(elementIndex, _) }
deprecated Expr getElementExpr(int elementIndex) { result = this.getElementExpr(elementIndex, _) }

@MathiasVP
Copy link
Contributor Author

What happens in this case?

Hmmm, good question. I'll add this test and see

@jketema
Copy link
Contributor

jketema commented Apr 4, 2023

Note that clang actually gives a warning on the slightly odd test case:

test.c:7:41: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
  struct Foo f = { .x = {0, 1}, .x[0] = 42, .y = 42 };
                                        ^~
test.c:7:26: note: previous initialization is here
  struct Foo f = { .x = {0, 1}, .x[0] = 42, .y = 42 };
                         ^
1 warning generated.

@MathiasVP
Copy link
Contributor Author

I've added the case in 4033ed3. The output actually looks sensible, I think 🤞.

@jketema
Copy link
Contributor

jketema commented Apr 4, 2023

I've added the case in 4033ed3. The output actually looks sensible, I think 🤞.

Looks sensible indeed.

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

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