Skip to content

[CIR][CodeGen] Removed special handling for array of union #1780

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tommymcm
Copy link
Contributor

@tommymcm tommymcm commented Aug 6, 2025

Special handling for array of unions was forcing all non-union C arrays to be emitted as CIR arrays, which differs from OG CodeGen, where array may be emitted as LLVM struct.

Closes #1315

Copy link
Collaborator

@andykaylor andykaylor left a comment

Choose a reason for hiding this comment

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

This looks good to me. The special handling for arrays of unions seems to have been off the mark.

@tommymcm
Copy link
Contributor Author

tommymcm commented Aug 7, 2025

Thanks for the review @andykaylor, I applied the requested changes.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Awesome!

Special handling for array of union was forcing all non-union C arrays to be emitted as CIR arrays, which differs from OG CodeGen, where some arrays are emitted as structs.
@tommymcm tommymcm force-pushed the revert-array-of-unions branch from c33200a to c060df4 Compare August 11, 2025 17:11
@tommymcm
Copy link
Contributor Author

Rebased on top of latest rebase.

@@ -1159,8 +1117,7 @@ class ConstExprEmitter

if (i == 0)
CommonElementType = C.getType();
else if (isDesiredArrayOfUnionDirectly &&
C.getType() != CommonElementType)
else if (C.getType() != CommonElementType)
Copy link
Member

Choose a reason for hiding this comment

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

It's worth adding a comment here to briefly mention how we deviate from OG (absence of isDesiredArrayOfUnionDirectly)

@bcardosolopes
Copy link
Member

I noticed a linux failure, is that related?

@tommymcm
Copy link
Contributor Author

I noticed a linux failure, is that related?

Looks like they're all errors in clang-tidy:

2025-08-11T17:27:56.2808094Z ********************
2025-08-11T17:27:56.2808303Z Failed Tests (81):
2025-08-11T17:27:56.2808585Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/0/82
2025-08-11T17:27:56.2809018Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/1/82
2025-08-11T17:27:56.2809424Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/10/82
2025-08-11T17:27:56.2809809Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/11/82
2025-08-11T17:27:56.2810204Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/12/82
2025-08-11T17:27:56.2810593Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/13/82
2025-08-11T17:27:56.2810984Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/14/82
2025-08-11T17:27:56.2811371Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/15/82
2025-08-11T17:27:56.2811764Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/16/82
2025-08-11T17:27:56.2812162Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/17/82
2025-08-11T17:27:56.2812549Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/18/82
2025-08-11T17:27:56.2812940Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/19/82
2025-08-11T17:27:56.2813327Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/2/82
2025-08-11T17:27:56.2813718Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/20/82
2025-08-11T17:27:56.2814231Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/21/82
2025-08-11T17:27:56.2814620Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/22/82
2025-08-11T17:27:56.2815022Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/23/82
2025-08-11T17:27:56.2815393Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/24/82
2025-08-11T17:27:56.2815760Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/25/82
2025-08-11T17:27:56.2816192Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/26/82
2025-08-11T17:27:56.2816562Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/27/82
2025-08-11T17:27:56.2816928Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/28/82
2025-08-11T17:27:56.2817286Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/29/82
2025-08-11T17:27:56.2817650Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/3/82
2025-08-11T17:27:56.2818006Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/30/82
2025-08-11T17:27:56.2818395Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/31/82
2025-08-11T17:27:56.2818984Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/32/82
2025-08-11T17:27:56.2819507Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/33/82
2025-08-11T17:27:56.2819880Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/34/82
2025-08-11T17:27:56.2820243Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/35/82
2025-08-11T17:27:56.2820609Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/36/82
2025-08-11T17:27:56.2821450Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/37/82
2025-08-11T17:27:56.2821834Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/38/82
2025-08-11T17:27:56.2822210Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/39/82
2025-08-11T17:27:56.2822573Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/4/82
2025-08-11T17:27:56.2822946Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/40/82
2025-08-11T17:27:56.2823314Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/41/82
2025-08-11T17:27:56.2823689Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/42/82
2025-08-11T17:27:56.2824229Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/43/82
2025-08-11T17:27:56.2824592Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/44/82
2025-08-11T17:27:56.2824971Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/45/82
2025-08-11T17:27:56.2825330Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/46/82
2025-08-11T17:27:56.2825689Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/47/82
2025-08-11T17:27:56.2826049Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/48/82
2025-08-11T17:27:56.2826458Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/49/82
2025-08-11T17:27:56.2826806Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/5/82
2025-08-11T17:27:56.2827177Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/50/82
2025-08-11T17:27:56.2827526Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/51/82
2025-08-11T17:27:56.2827877Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/52/82
2025-08-11T17:27:56.2828227Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/53/82
2025-08-11T17:27:56.2828566Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/54/82
2025-08-11T17:27:56.2828916Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/55/82
2025-08-11T17:27:56.2829259Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/56/82
2025-08-11T17:27:56.2829618Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/57/82
2025-08-11T17:27:56.2829977Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/58/82
2025-08-11T17:27:56.2830322Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/59/82
2025-08-11T17:27:56.2830668Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/6/82
2025-08-11T17:27:56.2831009Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/60/82
2025-08-11T17:27:56.2831359Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/61/82
2025-08-11T17:27:56.2831702Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/62/82
2025-08-11T17:27:56.2832042Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/63/82
2025-08-11T17:27:56.2832384Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/64/82
2025-08-11T17:27:56.2832720Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/65/82
2025-08-11T17:27:56.2833069Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/66/82
2025-08-11T17:27:56.2833416Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/67/82
2025-08-11T17:27:56.2833772Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/69/82
2025-08-11T17:27:56.2834227Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/7/82
2025-08-11T17:27:56.2834566Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/70/82
2025-08-11T17:27:56.2834913Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/71/82
2025-08-11T17:27:56.2835259Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/72/82
2025-08-11T17:27:56.2835598Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/73/82
2025-08-11T17:27:56.2835935Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/74/82
2025-08-11T17:27:56.2836316Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/75/82
2025-08-11T17:27:56.2836655Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/76/82
2025-08-11T17:27:56.2836990Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/77/82
2025-08-11T17:27:56.2837332Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/78/82
2025-08-11T17:27:56.2837880Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/79/82
2025-08-11T17:27:56.2838363Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/8/82
2025-08-11T17:27:56.2838711Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/80/82
2025-08-11T17:27:56.2839046Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/81/82
2025-08-11T17:27:56.2839385Z   Extra Tools Unit Tests :: clang-tidy/./ClangTidyTests/9/82
2025-08-11T17:27:56.2839595Z 
2025-08-11T17:27:56.2839600Z 
2025-08-11T17:27:56.2839673Z Testing Time: 18.17s
2025-08-11T17:27:56.2839799Z 
2025-08-11T17:27:56.2839882Z Total Discovered Tests: 2728
2025-08-11T17:27:56.2840104Z   Unsupported      :    7 (0.26%)
2025-08-11T17:27:56.2840329Z   Passed           : 2639 (96.74%)
2025-08-11T17:27:56.2840556Z   Expectedly Failed:    1 (0.04%)
2025-08-11T17:27:56.2840761Z   Failed           :   81 (2.97%)

I can look into it in the morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Const array of structs may have different element types
3 participants