llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Adam Smith (adams381) <details> <summary>Changes</summary> CallConvLowering recognizes a forwarded non-byval indirect argument by the slot its operand was loaded from. At -O1 and above cir-simplify folds that load away when the slot is a constant alloca, which is what CIRGen emits for a const-qualified by-value parameter. The walk that gives each such parameter's slot the alignment the ABI promises now also reads the record back out of the slot at the spill and points the parameter's call-argument uses at that load, so an Expand, byval or coerced argument reads it too. Assisted-by: Cursor / claude-opus-5 --- Patch is 61.41 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/225010.diff 8 Files Affected: - (modified) clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp (+12-9) - (modified) clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp (+147-12) - (modified) clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.h (+28-15) - (added) clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-thunk-mixed.cpp (+58) - (added) clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-thunk-sret.cpp (+70) - (added) clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-thunk.cpp (+50) - (modified) clang/test/CIR/Transforms/abi-lowering/indirect-non-byval-forward-param.cir (+655) - (modified) clang/test/CIR/Transforms/abi-lowering/indirect-non-byval-nyi.cir (+309) ``````````diff diff --git a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp index 3d878fd7866ef..9fd800c1e3178 100644 --- a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp +++ b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp @@ -889,7 +889,7 @@ void CallConvLoweringPass::runOnOperation() { DataLayout dl(moduleOp); CIRABIRewriteContext rewriteCtx(moduleOp, dl); // A non-byval indirect parameter's slot outlives the rewrite that retypes - // the parameter, so that a call forwarding the parameter can still recognise + // the parameter, so that a call forwarding the parameter can still recognize // it. Draining on scope exit collapses those slots whichever way this // function returns. llvm::scope_exit drainParamSlots( @@ -1032,14 +1032,17 @@ void CallConvLoweringPass::runOnOperation() { addressTakers[callee].push_back(getGlobal); }); - // Restate every non-byval indirect parameter's slot alignment as the one the - // ABI promises for that parameter, before anything reads a slot. A call is - // rewritten together with its callee rather than with the function - // containing it, so a call forwarding such a parameter can be reached before - // the parameter's own function is rewritten. Doing this up front makes the - // forwarding decision independent of the order the two were declared in. - for (auto &kv : classifications) - rewriteCtx.normalizeParameterSlotAlignments(kv.first, kv.second); + // Restate every non-byval indirect parameter's slot alignment and route any + // use of such a parameter as a call argument through a load of that slot, + // before any definition or call site is rewritten. Doing this up front + // makes the forwarding decision independent of the order the callee and its + // caller were declared in. + for (auto &kv : classifications) { + if (failed(rewriteCtx.prepareNonByvalParameters(kv.first, kv.second))) { + signalPassFailure(); + return; + } + } // Rewrite each function together with every direct call to it and every op // holding its address. By the time we move on to function F+1, F's diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp index 371fa67922872..e623f1cf94b0b 100644 --- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp +++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp @@ -559,11 +559,11 @@ static void eraseDeadRecordLoads(ArrayRef<cir::LoadOp> loads) { } /// The store that spills non-byval indirect parameter \p blockArg, and the -/// slot it spills into. CIRGen spills every by-value parameter into a local -/// alloca with a single store before any other use, and this pass runs on that -/// CIRGen output before any alloca-promoting or splitting pass, so the block -/// argument has exactly that one use. Both results are null when DCE already -/// removed a dead spill. +/// slot it spills into. prepareNonByvalParameters has already established +/// that the spill is the block argument's only use and that it stores into +/// an alloca, which is what the assertions below rest on. Both results are +/// null when the parameter has no spill, which is so only when nothing uses +/// it at all. static std::pair<cir::StoreOp, cir::AllocaOp> findParamSpill(mlir::BlockArgument blockArg) { if (blockArg.use_empty()) @@ -582,7 +582,7 @@ findParamSpill(mlir::BlockArgument blockArg) { /// 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 non-byval arg, change the block argument to a pointer and queue -/// the CIRGen param-slot alloca to be replaced by it (no entry load / +/// the param-slot alloca to be replaced by it (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 @@ -790,7 +790,7 @@ void insertArgCoercion( // Pointing the slot's uses at the incoming pointer waits until every // call site has been rewritten. A call that hands this parameter - // straight on recognises it by the slot its operand was loaded from, + // straight on recognizes it by the slot its operand was loaded from, // and collapsing the slot here would leave that call reading a block // argument with no defining operation to inspect. A dead spill DCE // already removed leaves nothing to collapse. @@ -1063,14 +1063,147 @@ void rewriteIndirectReturnCall(cir::CallOp call, } // namespace -void CIRABIRewriteContext::normalizeParameterSlotAlignments( +/// Bring \p funcOp's non-byval indirect parameter \p argNo into the shape the +/// rest of the rewrite assumes. \p claimedSlots carries the slots \p funcOp's +/// earlier non-byval indirect parameters took. See prepareNonByvalParameters. +static mlir::LogicalResult +prepareNonByvalParameter(cir::FuncOp funcOp, unsigned argNo, + const ArgClassification &ac, + mlir::BlockArgument blockArg, mlir::DominanceInfo &dom, + SmallPtrSetImpl<mlir::Operation *> &claimedSlots) { + // The spill is the store that writes the parameter itself. Any other use + // consumes the record value. At -O1 and above such a use comes from + // cir-simplify: CIRGen marks a const-qualified parameter's slot const, so + // the load of it folds to the stored parameter. + cir::StoreOp spill; + cir::StoreOp extraSpill; + mlir::Operation *otherUse = nullptr; + SmallVector<mlir::OpOperand *> callArgs; + for (mlir::OpOperand &use : blockArg.getUses()) { + auto store = dyn_cast<cir::StoreOp>(use.getOwner()); + if (store && store.getValue() == blockArg) { + // Which of two stores is the spill decides which slot stands in for the + // parameter, and the use list is in no particular order, so there is + // nothing to prefer between them. + if (spill) + extraSpill = store; + else + spill = store; + continue; + } + // Only an argument is served by a load of the slot. Any other consumer + // belongs to a rewrite that reads the parameter its own way: a returned + // record, for one, is rewritten through the sret slot, which assumes the + // returned load names the return slot and not this one. + auto call = dyn_cast<cir::CIRCallOpInterface>(use.getOwner()); + if (call && llvm::is_contained(call.getArgOperands(), blockArg)) + callArgs.push_back(&use); + else if (!otherUse) + otherUse = use.getOwner(); + } + + if (extraSpill) + return extraSpill->emitOpError() + << "non-byval parameter " << argNo + << " spilled more than once is not yet implemented in " + "CallConvLowering"; + + if (otherUse) + return otherUse->emitOpError() + << "non-byval parameter " << argNo + << " consumed other than as a call argument is not yet implemented " + "in CallConvLowering"; + + if (!spill) { + // Every other kind of use was reported above, so the parameter has no + // uses at all and needs neither a slot nor a read. + if (callArgs.empty()) + return mlir::success(); + + // Without a spill the parameter becomes the incoming pointer directly, + // which is the storage an argument taken from it has to name. Giving it + // the spill it lacks lets the checks and the read below apply unchanged, + // and neither survives the pass: insertArgCoercion erases the store and + // finalizeParameterSlots replaces the slot. + mlir::OpBuilder builder(funcOp.getContext()); + builder.setInsertionPointToStart(blockArg.getOwner()); + auto synthesized = cir::AllocaOp::create( + builder, funcOp.getLoc(), cir::PointerType::get(blockArg.getType()), + builder.getStringAttr("nonbyval.param"), + builder.getI64IntegerAttr(ac.indirectAlign.value())); + spill = + cir::StoreOp::create(builder, funcOp.getLoc(), blockArg, synthesized); + } + + // The incoming pointer replaces the slot itself, in the default address + // space. Retargeting a cast of it would leave the allocation's other views + // reading storage nothing writes. Storage in another address space, or + // storage that is not a local alloca at all, is not something the incoming + // pointer can be substituted for. + cir::AllocaOp slot = spill.getAddr().getDefiningOp<cir::AllocaOp>(); + if (!slot || + spill.getAddr().getType() != cir::PointerType::get(blockArg.getType())) + return spill->emitOpError() + << "non-byval parameter " << argNo + << " spilled to storage that cannot take the incoming pointer is " + "not yet implemented in CallConvLowering"; + + // One slot stands in for one non-byval parameter, since the incoming + // pointer replaces it. Two of them spilled to the same slot are each + // spilled once, so the check above cannot see the collision and only + // comparing the slots can. + if (!claimedSlots.insert(slot).second) + return spill->emitOpError() + << "non-byval parameter " << argNo + << " sharing its spill slot with another parameter is not yet " + "implemented in CallConvLowering"; + + // CIRGen picked the slot's alignment for a local copy of the record, but the + // slot is about to stand in for the parameter, and a call forwarding it may + // only promise what the incoming pointer does. + slot.setAlignment(ac.indirectAlign.value()); + + if (callArgs.empty()) + return mlir::success(); + + // A reader the spill does not dominate could read the slot before anything + // wrote it, and a load placed at the spill would not dominate it either. + for (mlir::OpOperand *callArg : callArgs) + if (!dom.properlyDominates(spill, callArg->getOwner())) + return callArg->getOwner()->emitOpError() + << "non-byval parameter " << argNo + << " read before its spill is not yet implemented in " + "CallConvLowering"; + + // Read the record back out of the slot instead, so every reader sees the + // shape CIRGen emits without cir-simplify: a load naming the storage the + // argument would have to name anyway. Reading at the spill, not at the + // reader, keeps the value the parameter's own whatever the body later + // stores into the slot, and the load promises only the alignment the + // classification gives the incoming pointer. + // + // A call that forwards the argument consumes the load once it recognizes + // the slot. One that wants a copy keeps it, reading the incoming pointer + // once finalizeParameterSlots replaces the slot. + mlir::OpBuilder builder(spill); + builder.setInsertionPointAfter(spill); + auto reload = cir::LoadOp::create(builder, spill.getLoc(), slot); + reload.setAlignment(ac.indirectAlign.value()); + for (mlir::OpOperand *callArg : callArgs) + callArg->set(reload.getResult()); + return mlir::success(); +} + +mlir::LogicalResult CIRABIRewriteContext::prepareNonByvalParameters( cir::FuncOp funcOp, const FunctionClassification &fc) { if (!funcOp.isDefinition()) - return; + return mlir::success(); mlir::Region &body = funcOp->getRegion(0); if (body.empty()) - return; + return mlir::success(); mlir::Block &entry = body.front(); + mlir::DominanceInfo dom; + SmallPtrSet<mlir::Operation *, 4> claimedSlots; // No signature has been rewritten yet, so no sret pointer has been prepended // and no Expand argument has been split into its fields. Every @@ -1081,9 +1214,11 @@ void CIRABIRewriteContext::normalizeParameterSlotAlignments( continue; assert(idx < entry.getNumArguments() && "classification count must not exceed entry block arguments"); - if (cir::AllocaOp slot = findParamSpill(entry.getArgument(idx)).second) - slot.setAlignment(ac.indirectAlign.value()); + if (failed(prepareNonByvalParameter(funcOp, idx, ac, entry.getArgument(idx), + dom, claimedSlots))) + return mlir::failure(); } + return mlir::success(); } void CIRABIRewriteContext::finalizeParameterSlots() { diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.h b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.h index b23359f8e5dee..c1b5468b80033 100644 --- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.h +++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.h @@ -66,22 +66,35 @@ class CIRABIRewriteContext : public mlir::abi::ABIRewriteContext { void rewriteFunctionAddress(cir::GetGlobalOp addrOp, cir::FuncOp funcOp, mlir::OpBuilder &builder); - /// Restate each non-byval indirect parameter's CIRGen slot alignment as the - /// alignment the ABI promises for that parameter. CIRGen picked the slot's - /// alignment for a local copy of the record, but the slot is about to stand - /// in for the parameter, and a call forwarding it may only promise what the - /// incoming pointer does. Call for every function before any call site is - /// rewritten, since a call is rewritten with its callee rather than with its - /// enclosing function and so may be reached first. Not an override, since - /// this has no counterpart in the generic contract. - void - normalizeParameterSlotAlignments(cir::FuncOp funcOp, - const mlir::abi::FunctionClassification &fc); + /// Bring each non-byval indirect parameter of \p funcOp into the shape the + /// rest of the rewrite assumes: the parameter's only use, if it has one, is + /// a single store into an alloca of the matching pointer type that no other + /// non-byval indirect parameter spills to, and that alloca states the + /// alignment the ABI promises rather than the one CIRGen picked for a local + /// copy. A use of the parameter as a call argument is routed through a load + /// of that slot, so that it names the storage an argument has to name. + /// + /// Call for every function before any definition or call site is rewritten. + /// findParamSpill asserts this shape while the enclosing definition is + /// rewritten, and a call is rewritten with its callee rather than with its + /// enclosing function and so may be reached first. + /// + /// A parameter read with no spill to name is given one, since it becomes + /// the incoming pointer directly. A parameter spilled twice, spilled to a + /// slot another such parameter also spills to, consumed other than as a + /// call argument, spilled where the incoming pointer cannot replace the + /// storage, or read where the spill does not dominate it gets a diagnostic + /// on \p funcOp and failure. + /// + /// Not an override, since this has no counterpart in the generic contract. + mlir::LogicalResult + prepareNonByvalParameters(cir::FuncOp funcOp, + const mlir::abi::FunctionClassification &fc); - /// Replace each non-byval indirect parameter's CIRGen slot with the + /// Replace each non-byval indirect parameter's spill slot with the /// incoming pointer, so the body operates on the caller's storage in place. /// Call once, after every function and call site has been rewritten: a call - /// forwarding such a parameter reads the slot to recognise it. Not an + /// forwarding such a parameter reads the slot to recognize it. Not an /// override, since deferring this has no counterpart in the generic /// contract. void finalizeParameterSlots(); @@ -92,10 +105,10 @@ class CIRABIRewriteContext : public mlir::abi::ABIRewriteContext { mlir::ModuleOp module; const mlir::DataLayout &dl; - /// CIRGen param-slot allocas that non-byval indirect parameters will + /// Param-slot allocas that non-byval indirect parameters will /// replace, paired with the incoming pointer that replaces them. The /// rewrite retypes the block argument but leaves the slot standing, because - /// a call site recognises a forwardable parameter by the slot its operand + /// a call site recognizes a forwardable parameter by the slot its operand /// was loaded from. finalizeParameterSlots does the replacement once every /// call site has been rewritten. llvm::SmallVector<std::pair<cir::AllocaOp, mlir::BlockArgument>> diff --git a/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-thunk-mixed.cpp b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-thunk-mixed.cpp new file mode 100644 index 0000000000000..6cc93986484ae --- /dev/null +++ b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-thunk-mixed.cpp @@ -0,0 +1,58 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -fclangir -emit-cir %s -o %t.cir +// RUN: FileCheck --check-prefix=CIR --input-file=%t.cir %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t-O0.cir +// RUN: FileCheck --check-prefix=CIR --input-file=%t-O0.cir %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -disable-llvm-passes -fclangir -emit-llvm %s -o %t-cir.ll +// RUN: FileCheck --check-prefixes=LLVM,LLVM-CIR --input-file=%t-cir.ll %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -O2 -disable-llvm-passes -emit-llvm %s -o %t.ll +// RUN: FileCheck --check-prefixes=LLVM,OGCG --input-file=%t.ll %s + +struct Str { + char c; + Str(const Str &); + ~Str(); +}; + +struct Io { + virtual char *stream(const Str &, Str); +}; + +struct Replicas { + virtual ~Replicas(); +}; + +struct Proxy : Replicas, Io { + ~Proxy(); +}; + +struct Namd : Proxy { + char *stream(const Str &, Str) override; +}; + +// The top-level const is written only on the definition, which is what marks +// the parameter's spill slot const. +char *Namd::stream(const Str &, const Str) { return nullptr; } + +// The by-value parameter is forwarded without a copy, from behind a reference +// parameter and alongside a returned pointer. + +// CIR-LABEL: cir.func{{.*}} @_ZThn8_N4Namd6streamERK3StrS0_ +// CIR-SAME: %{{[^ :]+}}: !cir.ptr<!rec_Namd> {llvm.noundef} +// CIR-SAME: %{{[^ :]+}}: !cir.ptr<!rec_Str> {llvm.align = 1 : i64, llvm.dereferenceable = 1 : i64, llvm.nonnull, llvm.noundef} +// CIR-SAME: %[[STR:[^ :]+]]: !cir.ptr<!rec_Str> {llvm.align = 1 : i64, llvm.dereferenceable = 1 : i64, llvm.nofreeobj, llvm.noundef} +// CIR: cir.call @_ZN4Namd6streamERK3StrS0_(%{{[^,)]+}}, %{{[^,)]+}}, %[[STR]]) : (!cir.ptr<!rec_Namd> {llvm.align = 8 : i64, llvm.dereferenceable = 16 : i64, llvm.nonnull, llvm.noundef}, !cir.ptr<!rec_Str> {llvm.align = 1 : i64, llvm.dereferenceable = 1 : i64, llvm.nonnull, llvm.noundef}, !cir.ptr<!rec_Str> {llvm.align = 1 : i64, llvm.dereferenceable = 1 : i64, llvm.nofreeobj, llvm.noundef}) -> (!cir.ptr<!s8i> {llvm.noundef}) + +// The overrider takes the same pointer without byval, which is the definition +// side of the same contract. + +// LLVM-LABEL: define dso_local noundef ptr @_ZN4Namd6streamERK3StrS0_( +// LLVM-SAME: ptr noundef nonnull align 8 dereferenceable(16) %{{[^,]+}}, +// LLVM-SAME: ptr noundef nonnull align 1 dereferenceable(1) %{{[^,]+}}, +// LLVM-SAME: ptr nofreeobj noundef align 1 dereferenceable(1) %{{[^,)]+}}) + +// LLVM-LABEL: define dso_local noundef ptr @_ZThn8_N4Namd6streamERK3StrS0_( +// LLVM-SAME: ptr noundef %{{[^,]+}}, +// LLVM-SAME: ptr noundef nonnull align 1 dereferenceable(1) %{{[^,]+}}, +// LLVM-SAME: ptr nofreeobj noundef align 1 dereferenceable(1) %[[STR:[^,)]+]]) +// LLVM-CIR: call noundef ptr @_ZN4Namd6streamERK3StrS0_(ptr noundef nonnull align 8 dereferenceable(16) %{{[^,)]+}}, ptr noundef nonnull align 1 dereferenceable(1) %{{[^,)]+}}, ptr nofreeobj noundef align 1 dereferenceable(1) %[[STR]]) +// OGCG: tail call noundef ptr @_ZN4Namd6streamERK3StrS0_(ptr noundef nonnull align 8 dereferenceable(16) %{{[^,)]+}}, ptr noundef nonnull align 1 dereferenceable(1) %{{[^,)]+}}, ptr nofreeobj noundef align 1 dereferenceable(1) %[[STR]]) diff --git a/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-non-byval-thunk-sret.cpp b/clang/test/CIR/CodeGen/call-conv-loweri... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/225010 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
