https://github.com/adams381 created https://github.com/llvm/llvm-project/pull/214983
Fix two x86_64 signatures that lower silently wrong code for hand-written CIR. x86_64 calling-convention lowering: 1. drops an array argument of three to eight eightbytes instead of passing it in memory. 2. lowers a callee's signature while leaving an indirect `cir.try_call` to that callee in the form it was written. The pass cannot assume a classification is safe to act on, so an `Ignore` that would drop a value still carrying data is now an error, for the return as well as an argument. The `try_call` reaches the call-site rewriter, which reports it because it cannot rebuild a `cir.try_call` yet. Assisted-by: Cursor / claude-opus-5 >From b1c80f41b6c694886d4996d2d8c6d2c8ccb2ccfb Mon Sep 17 00:00:00 2001 From: Adam Smith <[email protected]> Date: Sat, 8 Aug 2026 09:02:29 -0700 Subject: [PATCH] [CIR] Fix two x86_64 signatures that lowered silently wrong Fix two x86_64 signatures that lower silently wrong code for hand-written CIR. x86_64 calling-convention lowering: 1) drops an array argument of three to eight eightbytes instead of passing it in memory. 2) lowers a callee's signature while leaving an indirect cir.try_call to that callee in the form it was written. The pass cannot assume a classification is safe to act on, so an Ignore that would drop a value still carrying data is now an error, for the return as well as an argument. The try_call reaches the call-site rewriter, which reports it because it cannot rebuild a cir.try_call yet. Assisted-by: Cursor / claude-opus-5 --- .../Transforms/CallConvLoweringPass.cpp | 47 +++++++++---- .../indirect-call-classification-attr.cir | 45 ++++++++++++- .../abi-lowering/x86_64-array-drop-nyi.cir | 66 +++++++++++++++++++ .../abi-lowering/x86_64-empty-record.cir | 19 ++++++ .../x86_64-indirect-try-call-nyi.cir | 39 +++++++++++ .../abi-lowering/x86_64-struct-direct.cir | 10 +++ 6 files changed, 212 insertions(+), 14 deletions(-) create mode 100644 clang/test/CIR/Transforms/abi-lowering/x86_64-array-drop-nyi.cir create mode 100644 clang/test/CIR/Transforms/abi-lowering/x86_64-indirect-try-call-nyi.cir diff --git a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp index 193c2b6f4a9dc..13752edabec48 100644 --- a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp +++ b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp @@ -356,6 +356,25 @@ convertABIArgInfo(const llvm::abi::ArgInfo &info, MLIRContext *ctx, return ArgClassification::getIgnore(); } +/// Whether dropping \p ty from the signature would lose data. Ignore is the +/// ABI's answer for a void type and for an aggregate holding no data, and +/// dropping one of those is correct. A classification that would drop +/// anything else has to be reported instead, because nothing would be passed +/// in its place. +/// +/// An aggregate with no members counts as holding no data. That relies on +/// CIRGen only omitting a member that is itself empty, which is what makes an +/// over-aligned union of empty classes droppable here the way the ABI wants. +static bool ignoreLosesData(mlir::Type ty) { + if (!ty || isa<cir::VoidType>(ty)) + return false; + if (auto recTy = dyn_cast<cir::RecordType>(ty)) + return llvm::any_of(recTy.getMembers(), ignoreLosesData); + if (auto arrTy = dyn_cast<cir::ArrayType>(ty)) + return arrTy.getSize() != 0 && ignoreLosesData(arrTy.getElementType()); + return true; +} + /// Where \p fnTy's declared parameters end and its ellipsis arguments begin. /// /// The only x86_64 rule that reads this boundary sends an unnamed vector wider @@ -421,6 +440,17 @@ static std::optional<FunctionClassification> classifyX86_64Signature( << t; }; + // An Ignore the ABI does not mean, where rewriting the signature would pass + // nothing in the value's place. + auto nyiDrop = [&](const ArgClassification &ac, mlir::Type t, + llvm::StringRef what) { + if (ac.kind != ArgKind::Ignore || !ignoreLosesData(t)) + return false; + emitError() << "x86_64 calling-convention lowering would drop " << what + << " of type " << t << ", which is not yet implemented"; + return true; + }; + FunctionClassification fc; fc.returnsVoid = voidRet; mlir::Type origRet = voidRet ? mlir::Type() : retCIR; @@ -430,6 +460,8 @@ static std::optional<FunctionClassification> classifyX86_64Signature( nyiCoercion(retCIR); return std::nullopt; } + if (nyiDrop(*retAc, origRet, "the return value")) + return std::nullopt; fc.returnInfo = *retAc; for (unsigned i = 0, e = fi->arg_size(); i < e; ++i) { mlir::Type origArg = i < inputs.size() ? inputs[i] : mlir::Type(); @@ -439,6 +471,8 @@ static std::optional<FunctionClassification> classifyX86_64Signature( nyiCoercion(origArg); return std::nullopt; } + if (nyiDrop(*ac, origArg, "an argument")) + return std::nullopt; fc.argInfos.push_back(*ac); } return fc; @@ -750,17 +784,8 @@ void CallConvLoweringPass::runOnOperation() { // cached as the next one to visit. SmallVector<cir::CIRCallOpInterface> indirectCalls; moduleOp.walk([&](cir::CIRCallOpInterface c) { - cir::FuncType calleeTy = indirectCalleeType(c); - if (!calleeTy) - return; - // A cir.try_call is in this walk so that a variadic one reaches the - // ellipsis accounting below. CIRABIRewriteContext cannot rebuild a - // cir.try_call at all, so a non-variadic one has never been rewritten - // here. Keep it out rather than start reporting a gap that has nothing - // to do with the ellipsis. - if (!calleeTy.isVarArg() && isa<cir::TryCallOp>(c.getOperation())) - return; - indirectCalls.push_back(c); + if (indirectCalleeType(c)) + indirectCalls.push_back(c); }); for (cir::CIRCallOpInterface c : indirectCalls) { // classification-attr mode injects a per-function classification, which diff --git a/clang/test/CIR/Transforms/abi-lowering/indirect-call-classification-attr.cir b/clang/test/CIR/Transforms/abi-lowering/indirect-call-classification-attr.cir index 9c709d4ee8042..f9732688dcd03 100644 --- a/clang/test/CIR/Transforms/abi-lowering/indirect-call-classification-attr.cir +++ b/clang/test/CIR/Transforms/abi-lowering/indirect-call-classification-attr.cir @@ -42,9 +42,9 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<i32, dense<32>: vector<2xi64>>> } { - // A variadic indirect cir.try_call reaches the same driver-mode error. It is - // collected so that its ellipsis is accounted for, which a per-function - // injected classification cannot describe either. + // An indirect cir.try_call reaches the same driver-mode error, whether or + // not its callee is variadic, since a per-function injected classification + // cannot describe a callee resolved at run time either way. cir.func @variadic_try_caller(%fp: !cir.ptr<!cir.func<(!cir.ptr<!s8i>, ...) -> !s32i>>, %fmt: !cir.ptr<!s8i>, %n: !s32i) -> !s32i attributes { test_classify = #passthrough } { @@ -69,3 +69,42 @@ module attributes { } // CHECK: error: 'cir.try_call' op indirect call cannot be classified in the 'classification-attr' driver mode + +// ----- + +!s8i = !cir.int<s, 8> +!s32i = !cir.int<s, 32> +!void = !cir.void + +#passthrough = { + return = { kind = "direct" }, + args = [ { kind = "direct" }, { kind = "direct" } ] +} + +module attributes { + dlti.dl_spec = #dlti.dl_spec<#dlti.dl_entry<i32, dense<32>: vector<2xi64>>> +} { + + cir.func @try_caller(%fp: !cir.ptr<!cir.func<(!s32i) -> !s32i>>, %n: !s32i) + -> !s32i attributes { test_classify = #passthrough } { + %0 = cir.try_call %fp(%n) ^bb1, ^bb2 + : (!cir.ptr<!cir.func<(!s32i) -> !s32i>>, !s32i) -> !s32i + ^bb1: + cir.br ^bb4(%0 : !s32i) + ^bb2: + %1 = cir.eh.initiate : !cir.eh_token + cir.eh.dispatch %1 : !cir.eh_token [ + catch_all : ^bb3 + ] + ^bb3(%tok : !cir.eh_token): + %ct, %exn = cir.begin_catch %tok : !cir.eh_token -> (!cir.catch_token, !cir.ptr<!void>) + cir.end_catch %ct : !cir.catch_token + %2 = cir.const #cir.int<0> : !s32i + cir.br ^bb4(%2 : !s32i) + ^bb4(%r : !s32i): + cir.return %r : !s32i + } + +} + +// CHECK: error: 'cir.try_call' op indirect call cannot be classified in the 'classification-attr' driver mode diff --git a/clang/test/CIR/Transforms/abi-lowering/x86_64-array-drop-nyi.cir b/clang/test/CIR/Transforms/abi-lowering/x86_64-array-drop-nyi.cir new file mode 100644 index 0000000000000..fdcf5ce4d5a2f --- /dev/null +++ b/clang/test/CIR/Transforms/abi-lowering/x86_64-array-drop-nyi.cir @@ -0,0 +1,66 @@ +// RUN: not cir-opt %s -cir-call-conv-lowering=target=x86_64 \ +// RUN: -split-input-file 2>&1 | FileCheck %s + +!s64i = !cir.int<s, 64> + +module attributes { + cir.triple = "x86_64-unknown-linux-gnu", + dlti.dl_spec = #dlti.dl_spec< + #dlti.dl_entry<i8, dense<8>: vector<2xi64>>, + #dlti.dl_entry<i32, dense<32>: vector<2xi64>>, + #dlti.dl_entry<i64, dense<64>: vector<2xi64>>> +} { + + // The classifier answers Ignore for an array spanning three to eight + // eightbytes, where SysV says memory. Rewriting that answer would pass + // nothing in the value's place, so it is reported instead. Two eightbytes + // in registers and nine byval are both in x86_64-struct-direct.cir. + cir.func @take_arr_3eb(%arg0: !cir.array<!s64i x 3>) { + cir.return + } + + // CHECK: error: 'cir.func' op x86_64 calling-convention lowering would drop an argument of type '!cir.array<!cir.int<s, 64> x 3>', which is not yet implemented + + // Eight eightbytes is the last size the classifier answers Ignore for. + cir.func @take_arr_8eb(%arg0: !cir.array<!s64i x 8>) { + cir.return + } + + // CHECK: error: 'cir.func' op x86_64 calling-convention lowering would drop an argument of type '!cir.array<!cir.int<s, 64> x 8>', which is not yet implemented + + // The return value goes through the same guard and reports its own position. + cir.func @ret_arr_3eb() -> !cir.array<!s64i x 3> { + %0 = cir.alloca "r" align(8) : !cir.ptr<!cir.array<!s64i x 3>> + %1 = cir.load %0 : !cir.ptr<!cir.array<!s64i x 3>>, !cir.array<!s64i x 3> + cir.return %1 : !cir.array<!s64i x 3> + } + + // CHECK: error: 'cir.func' op x86_64 calling-convention lowering would drop the return value of type '!cir.array<!cir.int<s, 64> x 3>', which is not yet implemented +} + +// ----- + +// An indirect callee carries the array only in its pointee signature, so the +// report is anchored on the call rather than on the enclosing function. It +// needs its own module because a signature failure above stops the pass before +// the indirect-call walk. + +!s64i = !cir.int<s, 64> + +module attributes { + cir.triple = "x86_64-unknown-linux-gnu", + dlti.dl_spec = #dlti.dl_spec< + #dlti.dl_entry<i8, dense<8>: vector<2xi64>>, + #dlti.dl_entry<i32, dense<32>: vector<2xi64>>, + #dlti.dl_entry<i64, dense<64>: vector<2xi64>>> +} { + + cir.func @call_arr_3eb(%arg0: !cir.ptr<!cir.func<(!cir.array<!s64i x 3>)>>) { + %0 = cir.alloca "a" align(8) : !cir.ptr<!cir.array<!s64i x 3>> + %1 = cir.load %0 : !cir.ptr<!cir.array<!s64i x 3>>, !cir.array<!s64i x 3> + cir.call %arg0(%1) : (!cir.ptr<!cir.func<(!cir.array<!s64i x 3>)>>, !cir.array<!s64i x 3>) -> () + cir.return + } + + // CHECK: error: 'cir.call' op x86_64 calling-convention lowering would drop an argument of type '!cir.array<!cir.int<s, 64> x 3>', which is not yet implemented +} diff --git a/clang/test/CIR/Transforms/abi-lowering/x86_64-empty-record.cir b/clang/test/CIR/Transforms/abi-lowering/x86_64-empty-record.cir index 23254734f27e5..5dc3db0509ffd 100644 --- a/clang/test/CIR/Transforms/abi-lowering/x86_64-empty-record.cir +++ b/clang/test/CIR/Transforms/abi-lowering/x86_64-empty-record.cir @@ -61,4 +61,23 @@ module attributes { // CHECK: cir.func{{.*}} @caller_local(%arg0: !s32i) -> !s32i // CHECK: cir.alloca "e" align(1) : !cir.ptr<!rec_E0> // CHECK: cir.call @take_mixed(%arg0) : (!s32i) -> !s32i + + // A zero-length array holds no data, so Ignore is the ABI's answer and the + // argument is dropped rather than reported. A longer array of the same + // element type is reported instead, in x86_64-array-drop-nyi.cir. + cir.func @take_arr_empty(%arg0: !cir.array<!s32i x 0>) { + cir.return + } + + // CHECK: cir.func{{.*}} @take_arr_empty() + // CHECK-NEXT: cir.return + + // The same holds through a record, which is what the member walk answers + // for a wrapper whose fields are all empty. + cir.func @take_wrapped_empty(%arg0: !cir.struct<"WE" {!cir.array<!s32i x 0>, !rec_E0}>) { + cir.return + } + + // CHECK: cir.func{{.*}} @take_wrapped_empty() + // CHECK-NEXT: cir.return } diff --git a/clang/test/CIR/Transforms/abi-lowering/x86_64-indirect-try-call-nyi.cir b/clang/test/CIR/Transforms/abi-lowering/x86_64-indirect-try-call-nyi.cir new file mode 100644 index 0000000000000..c2866f0d2e869 --- /dev/null +++ b/clang/test/CIR/Transforms/abi-lowering/x86_64-indirect-try-call-nyi.cir @@ -0,0 +1,39 @@ +// RUN: not cir-opt %s -cir-call-conv-lowering=target=x86_64 2>&1 | FileCheck %s + +!s8i = !cir.int<s, 8> +!s32i = !cir.int<s, 32> +!void = !cir.void + +module attributes { + cir.triple = "x86_64-unknown-linux-gnu", + dlti.dl_spec = #dlti.dl_spec< + #dlti.dl_entry<i8, dense<8>: vector<2xi64>>, + #dlti.dl_entry<i32, dense<32>: vector<2xi64>>, + #dlti.dl_entry<i64, dense<64>: vector<2xi64>>> +} { + + // A narrow integer argument needs signext, so unlike @pass_through in + // x86_64-indirect-try-call.cir this call does not already carry its wire + // form. CIRABIRewriteContext cannot rebuild a cir.try_call, so it is + // reported. + cir.func @needs_rewrite(%arg0: !cir.ptr<!cir.func<(!s8i) -> !s32i>>, + %arg1: !s8i) { + %0 = cir.try_call %arg0(%arg1) ^bb1, ^bb2 + : (!cir.ptr<!cir.func<(!s8i) -> !s32i>>, !s8i) -> !s32i + ^bb1: + cir.br ^bb4 + ^bb2: + %1 = cir.eh.initiate : !cir.eh_token + cir.eh.dispatch %1 : !cir.eh_token [ + catch_all : ^bb3 + ] + ^bb3(%tok : !cir.eh_token): + %ct, %exn = cir.begin_catch %tok : !cir.eh_token -> (!cir.catch_token, !cir.ptr<!void>) + cir.end_catch %ct : !cir.catch_token + cir.br ^bb4 + ^bb4: + cir.return + } +} + +// CHECK: error: 'cir.try_call' op TryCallOp not yet implemented in CallConvLowering diff --git a/clang/test/CIR/Transforms/abi-lowering/x86_64-struct-direct.cir b/clang/test/CIR/Transforms/abi-lowering/x86_64-struct-direct.cir index 22cd3c68f2149..b46605ecea0c1 100644 --- a/clang/test/CIR/Transforms/abi-lowering/x86_64-struct-direct.cir +++ b/clang/test/CIR/Transforms/abi-lowering/x86_64-struct-direct.cir @@ -118,6 +118,15 @@ module attributes { // CHECK: cir.store %arg1, %[[E1]] : !s32i, !cir.ptr<!s32i> // CHECK: %{{.*}} = cir.cast bitcast %{{.*}} : !cir.ptr<!rec_anon_struct1> -> !cir.ptr<!cir.array<!s32i x 3>> + // Above eight eightbytes an array argument goes to memory. Seventy-two + // bytes here and twelve bytes in take_arr3 above are the sizes either side + // of the window x86_64-array-drop-nyi.cir reports. + cir.func @take_arr9(%arg0: !cir.array<!s64i x 9>) { + cir.return + } + + // CHECK: cir.func{{.*}} @take_arr9(%arg0: !cir.ptr<!cir.array<!s64i x 9>> {llvm.align = 8 : i64, llvm.byval = !cir.array<!s64i x 9>, llvm.noalias, llvm.noundef}) + // An anonymous struct has no record-layout entry, so it defaults to // can-pass-in-registers and coerces like its named counterpart. cir.func @take_anon(%arg0: !cir.struct<{!s32i, !s32i}>) { @@ -168,6 +177,7 @@ module attributes { // LLVM: define void @take_charbuf(i32 %{{.+}}) // LLVM: define void @take_char1(i8 %{{.+}}) // LLVM: define void @take_arr3(i64 %{{.+}}, i32 %{{.+}}) +// LLVM: define void @take_arr9(ptr noalias noundef byval([9 x i64]) align 8 %{{[^,)]+}}) // LLVM: define void @take_anon(i64 %{{.+}}) // LLVM: define void @call_pair(i64 %{{.+}}) // LLVM: %{{.+}} = call i32 @take_pair(i64 %{{.+}}) _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
