Skip to content

[CIR][Transforms] Fix CallConv Function Lowering #958

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

Closed
wants to merge 1 commit into from

Conversation

bruteforceboy
Copy link
Contributor

@bruteforceboy bruteforceboy commented Oct 10, 2024

Consider the following code snippet tmp.c:

typedef struct {
  int a, b;
} S;

void foo(S s) {}

Running bin/clang tmp.c -fclangir -Xclang -emit-llvm -Xclang -fclangir-call-conv-lowering -S -o -, we get:

loc(fused["tmp.c":5:1, "tmp.c":5:16]): error: 'llvm.bitcast' op result #0 must be LLVM-compatible non-aggregate type, but got '!llvm.struct<"struct.S", (i32, i32)>'

We can also produce a similar error from this:

typedef struct {
  int a, b;
} S;

S init() { S s; return s; }

gives:

loc(fused["tmp.c":5:17, "tmp.c":5:24]): error: 'llvm.bitcast' op operand #0 must be LLVM-compatible non-aggregate type, but got '!llvm.struct<"struct.S", (i32, i32)>'

I've traced the errors back to lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp in LowerFunction::buildAggregateStore, castReturnValue, and buildAggregateBitcast.

withElementType(SrcTy) is currently commented out/ignored in LowerFunction.cpp, but it is important.

This PR adds/fixes this and updates one test.

@sitio-couto
Copy link
Collaborator

Hey @bruteforceboy!

Could you double-check if you specified the target triple when testing?
This PR updates the test for the x86-64 ABI, but the original codegen example doesn't match what I'm seeing in Compiler Explorer—there's no LLVM IR bitcast here.

I added cir.bitcast in TargetLowering because CIR pointers aren't opaque like LLVM's; meaning the source value's type must match the target pointee's type. It's there to circumvent CIR's type-checking. Ideally, this bitcast should be dropped altogether when lowering to LLVM (something I forgot to do).

@bruteforceboy
Copy link
Contributor Author

Hey @bruteforceboy!

Could you double-check if you specified the target triple when testing? This PR updates the test for the x86-64 ABI, but the original codegen example doesn't match what I'm seeing in Compiler Explorer—there's no LLVM IR bitcast here.

I added cir.bitcast in TargetLowering because CIR pointers aren't opaque like LLVM's; meaning the source value's type must match the target pointee's type. It's there to circumvent CIR's type-checking. Ideally, this bitcast should be dropped altogether when lowering to LLVM (something I forgot to do).

You're right! I passed -fclangir to clang to produce the llvm above. If removing the bitcast fixes this, I can close the PR.

@bcardosolopes
Copy link
Member

Closing on behalf of @bruteforceboy, feel free to reopen if more work is going to be done here

@bruteforceboy
Copy link
Contributor Author

@sitio-couto Just to clarify. I just realized my clang version did not support opaque pointers, and that is why the LLVM IR above was produced.

bcardosolopes pushed a commit that referenced this pull request Oct 26, 2024
Re #958 

> Consider the following code snippet `tmp.c`: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> void foo(S s) {}
> ```
> Running `bin/clang tmp.c -fclangir -Xclang -emit-llvm -Xclang
-fclangir-call-conv-lowering -S -o -`, we get:
> ```
> loc(fused["tmp.c":5:1, "tmp.c":5:16]): error: 'llvm.bitcast' op result
#0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> We can also produce a similar error from this: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> S init() { S s; return s; }
> ```
> gives:
> ```
> loc(fused["tmp.c":5:17, "tmp.c":5:24]): error: 'llvm.bitcast' op
operand #0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> I've traced the errors back to
`lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp` in
`LowerFunction::buildAggregateStore`, `castReturnValue`, and
`buildAggregateBitcast`.
> 
> `withElementType(SrcTy)` is currently commented out/ignored in
`LowerFunction.cpp`, but it is important.
> 
> This PR adds/fixes this and updates one test.

I thought [about
it](#958 (comment))
and I understand adding `cir.bitcast` to circumvent the CIR checks, but
I am not sure how we can ignore/drop the bitcast while lowering. I think
we can just make the CIR casts correct.

I have added a number of lowering tests to verify that the CIR is
lowered properly.

cc: @sitio-couto @bcardosolopes.
lanza pushed a commit that referenced this pull request Nov 5, 2024
Re #958 

> Consider the following code snippet `tmp.c`: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> void foo(S s) {}
> ```
> Running `bin/clang tmp.c -fclangir -Xclang -emit-llvm -Xclang
-fclangir-call-conv-lowering -S -o -`, we get:
> ```
> loc(fused["tmp.c":5:1, "tmp.c":5:16]): error: 'llvm.bitcast' op result
#0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> We can also produce a similar error from this: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> S init() { S s; return s; }
> ```
> gives:
> ```
> loc(fused["tmp.c":5:17, "tmp.c":5:24]): error: 'llvm.bitcast' op
operand #0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> I've traced the errors back to
`lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp` in
`LowerFunction::buildAggregateStore`, `castReturnValue`, and
`buildAggregateBitcast`.
> 
> `withElementType(SrcTy)` is currently commented out/ignored in
`LowerFunction.cpp`, but it is important.
> 
> This PR adds/fixes this and updates one test.

I thought [about
it](#958 (comment))
and I understand adding `cir.bitcast` to circumvent the CIR checks, but
I am not sure how we can ignore/drop the bitcast while lowering. I think
we can just make the CIR casts correct.

I have added a number of lowering tests to verify that the CIR is
lowered properly.

cc: @sitio-couto @bcardosolopes.
lanza pushed a commit that referenced this pull request Mar 18, 2025
Re #958 

> Consider the following code snippet `tmp.c`: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> void foo(S s) {}
> ```
> Running `bin/clang tmp.c -fclangir -Xclang -emit-llvm -Xclang
-fclangir-call-conv-lowering -S -o -`, we get:
> ```
> loc(fused["tmp.c":5:1, "tmp.c":5:16]): error: 'llvm.bitcast' op result
#0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> We can also produce a similar error from this: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> S init() { S s; return s; }
> ```
> gives:
> ```
> loc(fused["tmp.c":5:17, "tmp.c":5:24]): error: 'llvm.bitcast' op
operand #0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> I've traced the errors back to
`lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp` in
`LowerFunction::buildAggregateStore`, `castReturnValue`, and
`buildAggregateBitcast`.
> 
> `withElementType(SrcTy)` is currently commented out/ignored in
`LowerFunction.cpp`, but it is important.
> 
> This PR adds/fixes this and updates one test.

I thought [about
it](#958 (comment))
and I understand adding `cir.bitcast` to circumvent the CIR checks, but
I am not sure how we can ignore/drop the bitcast while lowering. I think
we can just make the CIR casts correct.

I have added a number of lowering tests to verify that the CIR is
lowered properly.

cc: @sitio-couto @bcardosolopes.
xlauko pushed a commit to trailofbits/instafix-llvm that referenced this pull request Mar 28, 2025
Re llvm#958 

> Consider the following code snippet `tmp.c`: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> void foo(S s) {}
> ```
> Running `bin/clang tmp.c -fclangir -Xclang -emit-llvm -Xclang
-fclangir-call-conv-lowering -S -o -`, we get:
> ```
> loc(fused["tmp.c":5:1, "tmp.c":5:16]): error: 'llvm.bitcast' op result
#0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> We can also produce a similar error from this: 
> ```
> typedef struct {
>   int a, b;
> } S;
> 
> S init() { S s; return s; }
> ```
> gives:
> ```
> loc(fused["tmp.c":5:17, "tmp.c":5:24]): error: 'llvm.bitcast' op
operand #0 must be LLVM-compatible non-aggregate type, but got
'!llvm.struct<"struct.S", (i32, i32)>'
> ```
> I've traced the errors back to
`lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp` in
`LowerFunction::buildAggregateStore`, `castReturnValue`, and
`buildAggregateBitcast`.
> 
> `withElementType(SrcTy)` is currently commented out/ignored in
`LowerFunction.cpp`, but it is important.
> 
> This PR adds/fixes this and updates one test.

I thought [about
it](llvm/clangir#958 (comment))
and I understand adding `cir.bitcast` to circumvent the CIR checks, but
I am not sure how we can ignore/drop the bitcast while lowering. I think
we can just make the CIR casts correct.

I have added a number of lowering tests to verify that the CIR is
lowered properly.

cc: @sitio-couto @bcardosolopes.
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.

3 participants