https://github.com/adams381 created https://github.com/llvm/llvm-project/pull/210836
The CallConvLowering body rewrite for Indirect arguments treats byval and byref the same way: it inserts a cir.load at function entry and reroutes body uses to the loaded value. For byref — a non-trivially-copyable type passed by pointer — that entry load is a byte-copy, so the callee works on a local copy instead of the caller's storage. That breaks types whose representation embeds self-referential pointers: libstdc++'s SSO std::string keeps _M_p pointing at its own _M_local_buf, and a byte-copy leaves the copy's _M_p aliasing the source's buffer. This mirrors the sret return fix in insertSRetStores. For byref only, rewire the CIRGen param-slot alloca to the incoming pointer and drop the spill store, so the body operates on the caller's storage in place. byval keeps the load-at-entry copy, which is correct there. The rewrite is exercised through the classification-injection driver in clang/test/CIR/Transforms/abi-lowering/indirect-byval.cir, covering a byref field access on the incoming pointer and a byval control case that retains the copy. The call site still copies a value operand into a fresh alloca before passing it by reference; forwarding existing storage in place is left for a follow-up. >From 89634d9741325511d364cafda890fa0eef4984d2 Mon Sep 17 00:00:00 2001 From: Adam Smith <[email protected]> Date: Mon, 20 Jul 2026 15:49:42 -0700 Subject: [PATCH] [CIR] Rewire byref args to the incoming pointer Indirect byref previously shared byval's load-at-entry body rewrite, byte-copying the aggregate into a local slot. Non-trivially-copyable types (e.g. libstdc++ SSO std::string) must mutate the caller's storage in place, matching the sret path's alloca rewiring. Keep the entry load for byval, where a copy is correct. --- .../TargetLowering/CIRABIRewriteContext.cpp | 71 ++++++++++++++----- .../abi-lowering/indirect-byval.cir | 66 +++++++++++++++-- 2 files changed, 116 insertions(+), 21 deletions(-) diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp index 9fd1835243deb..b21e762895331 100644 --- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp +++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp @@ -23,7 +23,10 @@ using namespace mlir::abi; // For byval (ArgClassification::byVal == true) the callee gets // llvm.byval + llvm.noalias + llvm.noundef; for byref (byVal == false) // the callee gets llvm.byref without the ownership attrs. Both pass -// through an alloca+store at the call site. +// through an alloca+store at the call site. At the callee, byval loads +// the incoming pointer (a local copy), while byref rewires the CIRGen +// param-slot alloca to the incoming pointer so the body mutates the +// caller's storage in place. // // For Expand, the single struct argument is replaced by N scalar arguments // (one per field). At the callee, the N field block arguments are stored @@ -437,9 +440,12 @@ emitStructFieldArgs(mlir::OpBuilder &builder, mlir::Location loc, /// For each Direct arg with a coerced type, change the block argument's type /// to the coerced type and insert a coercion at function entry that maps it -/// back to the original type for body uses. For each Indirect (byval/byref) -/// arg, change the block argument's type to a pointer and insert a load at -/// entry so the body sees the original value type. For each Expand arg, +/// back to the original type for body uses. For each Indirect byval arg, +/// change the block argument's type to a pointer and insert a load at entry +/// so the body sees a local copy of the original value type. For each +/// Indirect byref arg, change the block argument to a pointer and rewire the +/// CIRGen param-slot alloca to that pointer (no entry load / byte-copy) so +/// the body operates on the caller's storage in place. For each Expand arg, /// replace the single struct block argument with N scalar block arguments (one /// per field) and store each field directly into the parameter's own alloca /// (the CIRGen spill slot), erasing the original whole-struct store. @@ -619,19 +625,52 @@ void insertArgCoercion(mlir::FunctionOpInterface funcOp, // to adapted (now of the original type != the alloca's pointee type). blockArg.replaceAllUsesExcept(adapted, coercionOps); } else if (ac.kind == ArgKind::Indirect) { - // byval and byref: the wire type is !cir.ptr<T>. Change the block arg - // to the pointer type and insert a load so the body sees the original - // T. The body transformation is the same for both; the distinction - // between byval (llvm.byval) and byref (llvm.byref) is in the arg - // attributes applied by updateArgAttrs. - mlir::Type origTy = blockArg.getType(); - auto ptrTy = cir::PointerType::get(origTy); - blockArg.setType(ptrTy); + // byval and byref share a !cir.ptr<T> wire type; the llvm.byval vs + // llvm.byref distinction is in the attrs applied by updateArgAttrs. + // Body lowering differs: byval copies into the callee (load at entry), + // while byref must operate on the caller's storage in place. + auto ptrTy = cir::PointerType::get(blockArg.getType()); + + if (!ac.byVal) { + // byref: CIRGen spills every by-value parameter into a local alloca + // with a single store before any other use. Rewire that alloca to + // the incoming pointer and drop the store, mirroring insertSRetStores + // for non-trivially-copyable aggregates (e.g. libstdc++ SSO + // std::string, where a byte-copy would leave `_M_p` aliasing the + // source's `_M_local_buf`). DCE may have removed a dead spill; + // tolerate that by only retyping the block argument. + cir::StoreOp paramStore; + cir::AllocaOp destAlloca; + if (!blockArg.use_empty()) { + assert(blockArg.hasOneUse() && + "byref arg must have exactly one use (the CIRGen param " + "spill)"); + paramStore = cast<cir::StoreOp>(*blockArg.user_begin()); + assert(paramStore.getValue() == blockArg && + "byref arg's use must be the value operand of its store"); + destAlloca = + cast<cir::AllocaOp>(paramStore.getAddr().getDefiningOp()); + } - builder.setInsertionPointToStart(&entry); - auto loadOp = cir::LoadOp::create(builder, funcOp.getLoc(), blockArg); - SmallPtrSet<mlir::Operation *, 1> loadOps = {loadOp}; - blockArg.replaceAllUsesExcept(loadOp.getResult(), loadOps); + if (paramStore) + paramStore->erase(); + + blockArg.setType(ptrTy); + + if (destAlloca) { + destAlloca.getResult().replaceAllUsesWith(blockArg); + destAlloca->erase(); + } + } else { + // byval: load the incoming pointer so the body sees a T value (and + // any CIRGen param-slot store becomes a local copy of that value). + blockArg.setType(ptrTy); + + builder.setInsertionPointToStart(&entry); + auto loadOp = cir::LoadOp::create(builder, funcOp.getLoc(), blockArg); + SmallPtrSet<mlir::Operation *, 1> loadOps = {loadOp}; + blockArg.replaceAllUsesExcept(loadOp.getResult(), loadOps); + } } // Ignore, Extend, and Direct-without-coerce need no block-level changes. diff --git a/clang/test/CIR/Transforms/abi-lowering/indirect-byval.cir b/clang/test/CIR/Transforms/abi-lowering/indirect-byval.cir index f0a8572403fe9..887d8f5b0d7cb 100644 --- a/clang/test/CIR/Transforms/abi-lowering/indirect-byval.cir +++ b/clang/test/CIR/Transforms/abi-lowering/indirect-byval.cir @@ -196,13 +196,15 @@ module attributes { // CHECK: cir.store %{{.*}}, %{{.*}} : !rec_Big, !cir.ptr<!rec_Big> // CHECK: cir.return - // byref callee definition: the parameter gets llvm.byref instead of - // llvm.byval. noalias and noundef are NOT added (byref passes the - // original, which may alias and may carry padding). The body - // transformation (ptr + load at entry) is identical to byval. + // byref callee: llvm.byref without noalias/noundef. CIRGen spills the + // by-value param into a local alloca; the rewriter rewires that alloca to + // the incoming pointer (no entry load / byte-copy). cir.func @takes_big_byref(%arg0: !rec_Big) -> !rec_Big attributes { test_classify = #byref_arg } { - cir.return %arg0 : !rec_Big + %0 = cir.alloca "arg0" align(8) init : !cir.ptr<!rec_Big> + cir.store %arg0, %0 : !rec_Big, !cir.ptr<!rec_Big> + %1 = cir.load %0 : !cir.ptr<!rec_Big>, !rec_Big + cir.return %1 : !rec_Big } // CHECK: cir.func{{.*}} @takes_big_byref(%[[PTR:.*]]: !cir.ptr<!rec_Big> @@ -210,9 +212,63 @@ module attributes { // CHECK-SAME: llvm.byref = !rec_Big // CHECK-NOT: llvm.noalias // CHECK-NOT: llvm.noundef + // CHECK-NOT: cir.alloca // CHECK: %[[L:.*]] = cir.load %[[PTR]] : !cir.ptr<!rec_Big>, !rec_Big // CHECK-NEXT: cir.return %[[L]] : !rec_Big + // byref field access must use the incoming pointer in place — no local + // alloca and no byte-copy of the non-trivially-copyable aggregate. + cir.func @takes_big_byref_field(%arg0: !rec_Big) -> !s64i + attributes { test_classify = #byref_arg } { + %0 = cir.alloca "arg0" align(8) init : !cir.ptr<!rec_Big> + cir.store %arg0, %0 : !rec_Big, !cir.ptr<!rec_Big> + %1 = cir.get_member %0[0] {name = "a"} : !cir.ptr<!rec_Big> -> !cir.ptr<!s64i> + %2 = cir.load %1 : !cir.ptr<!s64i>, !s64i + cir.return %2 : !s64i + } + + // CHECK: cir.func{{.*}} @takes_big_byref_field(%[[PTR:.*]]: !cir.ptr<!rec_Big> + // CHECK-SAME: llvm.byref = !rec_Big + // CHECK-NOT: cir.alloca + // CHECK: %[[M:.*]] = cir.get_member %[[PTR]][0] {name = "a"} : !cir.ptr<!rec_Big> -> !cir.ptr<!s64i> + // CHECK-NEXT: %[[V:.*]] = cir.load %[[M]] : !cir.ptr<!s64i>, !s64i + // CHECK-NEXT: cir.return %[[V]] : !s64i + + // byval with the same CIRGen spill keeps a local copy: load the byval + // pointer at entry and store into the param-slot alloca. + cir.func @takes_big_byval_field(%arg0: !rec_Big) -> !s64i + attributes { test_classify = #byval_arg } { + %0 = cir.alloca "arg0" align(8) init : !cir.ptr<!rec_Big> + cir.store %arg0, %0 : !rec_Big, !cir.ptr<!rec_Big> + %1 = cir.get_member %0[0] {name = "a"} : !cir.ptr<!rec_Big> -> !cir.ptr<!s64i> + %2 = cir.load %1 : !cir.ptr<!s64i>, !s64i + cir.return %2 : !s64i + } + + // CHECK: cir.func{{.*}} @takes_big_byval_field(%[[PTR:.*]]: !cir.ptr<!rec_Big> + // CHECK-SAME: llvm.byval = !rec_Big + // CHECK: %[[LOADED:.*]] = cir.load %[[PTR]] : !cir.ptr<!rec_Big>, !rec_Big + // CHECK: %[[SLOT:.*]] = cir.alloca "arg0" align(8) init : !cir.ptr<!rec_Big> + // CHECK: cir.store %[[LOADED]], %[[SLOT]] : !rec_Big, !cir.ptr<!rec_Big> + // CHECK: %[[M:.*]] = cir.get_member %[[SLOT]][0] {name = "a"} : !cir.ptr<!rec_Big> -> !cir.ptr<!s64i> + // CHECK: %[[V:.*]] = cir.load %[[M]] : !cir.ptr<!s64i>, !s64i + // CHECK: cir.return %[[V]] : !s64i + + // byref with the param spill removed (e.g. by DCE): the block arg has no + // use, so the rewriter only retypes it to a pointer and emits nothing else. + cir.func @takes_big_byref_unused(%arg0: !rec_Big) -> !s32i + attributes { test_classify = #byref_arg } { + %r = cir.const #cir.int<0> : !s32i + cir.return %r : !s32i + } + + // CHECK: cir.func{{.*}} @takes_big_byref_unused(%{{.*}}: !cir.ptr<!rec_Big> + // CHECK-SAME: llvm.byref = !rec_Big + // CHECK-NOT: cir.alloca + // CHECK-NOT: cir.load + // CHECK: %[[R:.*]] = cir.const #cir.int<0> : !s32i + // CHECK-NEXT: cir.return %[[R]] : !s32i + // byref forward declaration: signature gets llvm.byref, no body. cir.func private @takes_big_byref_decl(%arg0: !rec_Big) -> !rec_Big attributes { test_classify = #byref_arg } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
