Skip to content
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

Lowering to LLVM fails for constant array of unions with unequal initializer types #1185

Open
bruteforceboy opened this issue Nov 28, 2024 · 7 comments · May be fixed by #1236
Open

Lowering to LLVM fails for constant array of unions with unequal initializer types #1185

bruteforceboy opened this issue Nov 28, 2024 · 7 comments · May be fixed by #1236

Comments

@bruteforceboy
Copy link
Contributor

In, #1166, is there a reason why setting CommonElementType = nullptr was removed when all the elements don't have the same type?

For example, the following code snippet now fails when lowering to LLVM:

typedef struct {
  char a;
} S_1;

typedef struct {
  long a, b;
} S_2;

typedef union {
  S_1 a;
  S_2 b;
} U;

void foo() { U arr[2] = {{.b = {1, 2}}, {.a = {1}}}; }

cc: @bcardosolopes, @ChuanqiXu9

@bruteforceboy bruteforceboy changed the title Lowering to LLVM fails for constant array of unions with unequal types Lowering to LLVM fails for constant array of unions with unequal initializer types Nov 28, 2024
@ChuanqiXu9
Copy link
Member

ChuanqiXu9 commented Nov 28, 2024

The previous use case was, we hope the type of array of struct with unions to be the same. However, it looks like the array of unions are an outliner.

To be more clear, it is about array of initializers for unions (or containing unions). But now we can't express the type for unions (or for type containing unions) precisely, this is the intention of #1007

I am wondering if we can add an attribute to constStructAttr and add a "isTypeAttributesCompatible" method, then we can use it to compare the CommonElementType. So although it can't solve the fundamental problem I described before as two type systems, I feel it can solve the particular problem here.

@bcardosolopes
Copy link
Member

I am wondering if we can add an attribute to constStructAttr

Indeed, looks like we need some extra information on the attributes for the sake of unions.

@bruteforceboy
Copy link
Contributor Author

Hello @ChuanqiXu9, @bcardosolopes any updates regarding this issue?)

@ChuanqiXu9
Copy link
Member

Hello @ChuanqiXu9, @bcardosolopes any updates regarding this issue?)

Sorry. Quite busy with other business. I'll try if I can find sometime next week to look into the details. Or if you have time, maybe you can try it with above suggestions.

@bruteforceboy
Copy link
Contributor Author

I would appreciate if you have the time next week since you have more knowledge about the issue, but I would also try with your suggestions. Thanks for the quick reply)

@bcardosolopes
Copy link
Member

I might be able to take a look next week but more likely next year, sorry!

ChuanqiXu9 added a commit to ChuanqiXu9/clangir that referenced this issue Dec 16, 2024
Close llvm#1185

The patch itself seems slightly ad-hoc. As the issue tracked by
llvm#1178, the fundamental solution
may be to introduce two type systems to solve the inconsistent semantics
for Union between LLVM IR and CIR. This will be great to handle other
inconsistent semantics between LLVM IR and CIR if any.

Back to the patch itself, although the code looks not good initially to
me too. But I feel it may be a good workaround since
clang/test/CIR/Lowering/union-array.c is an example for array of unions
directly and clang/test/CIR/Lowering/nested-union-array.c gives an
example for array of unions indirectly (array of structs which contain
unions). So I feel we've already covered all the cases.

And generally it should be good to use some simple and solid workaround
before we introduce the formal full solution.
@ChuanqiXu9
Copy link
Member

Sent #1236 and see the comments there for details.

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 a pull request may close this issue.

3 participants