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

bruteforceboy · 2024-10-15T09:47:40Z

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 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.

smeenai · 2024-10-15T19:34:13Z

Thanks! This is marked as a draft, be sure to mark it ready for review when you want us to take a look :)

bruteforceboy · 2024-10-16T14:52:43Z

Thanks! This is marked as a draft, be sure to mark it ready for review when you want us to take a look :)

Yes, there are a few things I want to check. It should be ready for review soon.

sitio-couto · 2024-10-17T21:52:35Z

Hey @bruteforceboy!

I am not sure how we can ignore/drop the bitcast while lowering.

Since this workaround for type-checking is likely to show up in the future, this is what I had in mind:

Create a new CastKind called type_check_bypass.
Update CC lowering to use this new CastKind in this case.
Drop CastOps of kind type_check_bypass when lowering from CIR to LLVM Dialect.

This explicitly marks the intent of these kinds of casts, doesn't require much code, and makes it easy to drop the cast.
@bcardosolopes also suggested a separate operation for this use case, which semantically makes more sense, I think.

I think we can just make the CIR casts correct.

Looking forward to seeing what you cook up!

bcardosolopes · 2024-10-17T22:17:02Z

@sitio-couto thanks for the feedback, sounds reasonable.

Create a new CastKind called type_check_bypass.

Maybe call it abi or something related to it's actual intent.

bruteforceboy · 2024-10-18T10:26:52Z

@sitio-couto creating a new cast kind sounds good! Personally, I played around with trying to remove the bitcast, but I didn't make so much progress. I had several problems with mismatched types after removing the bitcasts.

I have now updated the PR, with my attempt at making the CIR correct. I have compared the generated LLVM, with the original codegen, and everything seems fine.

bcardosolopes

This is great, thanks for working on it

bcardosolopes · 2024-10-18T18:35:53Z

clang/test/CIR/Lowering/struct.c

@@ -0,0 +1,89 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t.ll -fclangir-call-conv-lowering
+// RUN: FileCheck --input-file=%t.ll %s -check-prefix=LLVM


This test should be in clang/test/CIR/Transforms/Target/x86_64/

bcardosolopes · 2024-10-18T18:36:18Z

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp

+      auto Dest = storeOp.getAddr();
+      auto DestPtr = emitPointerBitcast(Dest, SrcTy, *this);
+      rewriter.replaceOpWithNewOp<StoreOp>(storeOp, Ret, DestPtr);
+    }


What happens if this isn't a StoreOp?

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp

bcardosolopes · 2024-10-18T18:38:59Z

clang/test/CIR/Lowering/struct.c

+  int a, b;
+} S;
+
+// LLVM: @init(i64 %[[#V0:]])


This testcase is great, thanks. The test should also show post-lowering in CIR terms, see other tests from clang/test/CIR/Transforms/Target/x86_64/, most important it should show the casts.

Noted. I will update these next week.

I would advise against these tests.

They are too monolithic as they test multiple areas of the pipeline and are not quite related to the changes in this PR. These usually are extra work to maintain.

The CC lowering test already covers what this PR changes, what comes after is another pass' responsibility.

I'm talking about the location, I think this is the best subdir to use for callconv tests, @sitio-couto do you have an alternative suggestion?

The CC lowering test already covers what this PR changes, what comes after is another pass' responsibility.

It's totally fine if new tests in this subdir test for LLVM output

@bcardosolopes I see what you mean about the location, and I agree that from the options we have, your suggestion is the best. IIUC, these are integration tests that aim to check if ClangIR meets its LLVM IR baseline requirement, do we have a folder for those?

The CC lowering tests already feel a bit too monolithic. Ideally, they should focus on CIR-to-CIR testing, while CIR-to-LLVM is handled by lowering tests. C/C++ to LLVM IR is another suite of tests of its own, I suppose.

I agree with @sitio-couto that we should keep CIR-to-CIR testing under transform tests, and LLVM in lowering tests. For now, I will add the CIR equivalent under the transform tests, and leave the LLVM equivalent under the lowering tests. Let me know what you think.

Ideally, they should focus on CIR-to-CIR testing, while CIR-to-LLVM is handled by lowering tests. C/C++ to LLVM IR is another suite of tests of its own, I suppose.

In an ideal world that would be right, but we need AST for some of the pieces and we don't yet have a mechanism to serialize them, so CIR to CIR is only applicable to a handful of cases. Note that test/clang/CIR/Lowering is already a mix of testing both LLVM and CIR in the same file AND standalone CIR tests. We should follow this pattern until we have a AST serialization story that will break the monolithic behavior for good.

For now, I will add the CIR equivalent under the transform tests, and leave the LLVM equivalent under the lowering tests. Let me know what you think.

I just moved things around and we now have test/clang/CIR/CallConvLowering/AArch64, please put struct.c there and test both CIR and LLVM output in the same file (note that this is a very common pattern in Lowering subdir).

we need AST for some of the pieces [...] so CIR to CIR is only applicable to a handful of cases.

Agreed, and in this case, it applies: the test doesn't rely on AST info. While these tests are necessary, we should minimize them when possible, as you pointed out, it's the "ideal."

test/clang/CIR/Lowering is already a mix of testing both LLVM and CIR in the same file AND standalone CIR tests. We should follow this pattern until we have a AST serialization story that will break the monolithic behavior for good.

I think we should follow said pattern where it makes sense; otherwise, we'll introduce unnecessary redundancy. Lowering tests are tightly coupled with LLVM Dialect and IR, requiring both cir-opt and cir-translate, so it's coherent to include both. This approach is also common in CodeGen, to ensure CIRGen emits the same LLVM IR as CodeGen (AFAIK), which also seems coherent. But for the CC lowering tests, it seems unnecessary.

In any case, I'm fine with your call.

I moved the tests to test/clang/CIR/CallConvLowering/AArch64.

bcardosolopes · 2024-10-18T18:42:51Z

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp

@@ -899,6 +909,15 @@ Value LowerFunction::rewriteCallOp(const LowerFunctionInfo &CallInfo,
    }
  }();

+  for (auto user : Caller.getOperation()->getUsers()) {


Seems like updating the users of a transformed call should be a common thing to do in target lowering, I wonder if there's a better place for this or if the logic can be incorporated in a pre-existing place. cc @sitio-couto

This seems hacky. You should not have to handle more bit casts after the return value lambda that does the type coercion, is already concluded. Especially not in a path common to all return types and all store ops. This should be done only to struct return values and their respective stores.

This should be within the createCoercedValue call you left commented above. Please move this logic there.

Was this fixed? Seems like I'm seeing the same code?

@bruteforceboy please allow the reviewers to resolve conversations, easier to make sure things got addressed.

oh, sorry about that. I moved the rewrite for the store operations, so it is only for struct return values.

sitio-couto

@bruteforceboy reviewed.

clang/test/CIR/Transforms/Target/x86_64/x86_64-call-conv-lowering-pass.cpp

sitio-couto · 2024-10-18T20:02:40Z

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp

@@ -899,6 +909,15 @@ Value LowerFunction::rewriteCallOp(const LowerFunctionInfo &CallInfo,
    }
  }();

+  for (auto user : Caller.getOperation()->getUsers()) {


This seems hacky. You should not have to handle more bit casts after the return value lambda that does the type coercion, is already concluded. Especially not in a path common to all return types and all store ops. This should be done only to struct return values and their respective stores.

This should be within the createCoercedValue call you left commented above. Please move this logic there.

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp

sitio-couto · 2024-10-18T20:30:49Z

clang/test/CIR/Lowering/struct.c

+  int a, b;
+} S;
+
+// LLVM: @init(i64 %[[#V0:]])


I would advise against these tests.

They are too monolithic as they test multiple areas of the pipeline and are not quite related to the changes in this PR. These usually are extra work to maintain.

The CC lowering test already covers what this PR changes, what comes after is another pass' responsibility.

clang/lib/CIR/Dialect/Transforms/TargetLowering/LowerFunction.cpp

bcardosolopes · 2024-10-21T19:23:06Z

@sitio-couto any else that needs addressing?

sitio-couto

@bruteforceboy approved! thanks for changes.

@sitio-couto

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.

bruteforceboy requested review from lanza and bcardosolopes as code owners October 15, 2024 09:47

bruteforceboy marked this pull request as draft October 15, 2024 11:45

bruteforceboy marked this pull request as ready for review October 18, 2024 10:26

smeenai mentioned this pull request Oct 18, 2024

Calling Convention Lowering: support function pointers #995

Closed

bcardosolopes requested changes Oct 18, 2024

View reviewed changes

bcardosolopes requested a review from sitio-couto October 18, 2024 18:44

sitio-couto requested changes Oct 18, 2024

View reviewed changes

lanza force-pushed the main branch from 5b99f59 to cb0cb34 Compare October 19, 2024 00:42

bruteforceboy closed this Oct 21, 2024

bruteforceboy force-pushed the call-conv branch from dc6b5f4 to e744086 Compare October 21, 2024 07:43

merge

ff34efb

bruteforceboy reopened this Oct 21, 2024

bruteforceboy added 2 commits October 21, 2024 15:26

review fixes

2886ec3

fix failing tests

2a0d3c9

sitio-couto approved these changes Oct 21, 2024

View reviewed changes

bruteforceboy added 2 commits October 22, 2024 12:59

merge

c02ac17

move tests to CallConvLowering/AArch64

fd9cf52

bcardosolopes approved these changes Oct 26, 2024

View reviewed changes

bcardosolopes merged commit 33857b4 into llvm:main Oct 26, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bruteforceboy commented Oct 15, 2024 •

edited

Loading

smeenai commented Oct 15, 2024

bruteforceboy commented Oct 16, 2024

sitio-couto commented Oct 17, 2024

bcardosolopes commented Oct 17, 2024 •

edited

Loading

bruteforceboy commented Oct 18, 2024

bcardosolopes left a comment

bcardosolopes Oct 18, 2024

bcardosolopes Oct 18, 2024

bcardosolopes Oct 18, 2024 •

edited

Loading

bruteforceboy Oct 18, 2024

sitio-couto Oct 18, 2024

bcardosolopes Oct 18, 2024

sitio-couto Oct 18, 2024 •

edited

Loading

bruteforceboy Oct 21, 2024 •

edited

Loading

bcardosolopes Oct 21, 2024

sitio-couto Oct 21, 2024 •

edited

Loading

bruteforceboy Oct 22, 2024

bcardosolopes Oct 18, 2024

sitio-couto Oct 18, 2024 •

edited

Loading

bcardosolopes Oct 21, 2024

bruteforceboy Oct 21, 2024

sitio-couto left a comment

sitio-couto Oct 18, 2024 •

edited

Loading

sitio-couto Oct 18, 2024

bcardosolopes commented Oct 21, 2024

sitio-couto left a comment

		@@ -0,0 +1,89 @@
		// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t.ll -fclangir-call-conv-lowering
		// RUN: FileCheck --input-file=%t.ll %s -check-prefix=LLVM

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

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

Conversation

bruteforceboy commented Oct 15, 2024 • edited Loading

smeenai commented Oct 15, 2024

bruteforceboy commented Oct 16, 2024

sitio-couto commented Oct 17, 2024

bcardosolopes commented Oct 17, 2024 • edited Loading

bruteforceboy commented Oct 18, 2024

bcardosolopes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sitio-couto Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

bruteforceboy Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sitio-couto Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sitio-couto Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sitio-couto left a comment

Choose a reason for hiding this comment

sitio-couto Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcardosolopes commented Oct 21, 2024

sitio-couto left a comment

Choose a reason for hiding this comment

bruteforceboy commented Oct 15, 2024 •

edited

Loading

bcardosolopes commented Oct 17, 2024 •

edited

Loading

bcardosolopes Oct 18, 2024 •

edited

Loading

sitio-couto Oct 18, 2024 •

edited

Loading

bruteforceboy Oct 21, 2024 •

edited

Loading

sitio-couto Oct 21, 2024 •

edited

Loading

sitio-couto Oct 18, 2024 •

edited

Loading

sitio-couto Oct 18, 2024 •

edited

Loading