llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Chaitanya (skc7) <details> <summary>Changes</summary> **Summary:** Stacked on PR #<!-- -->229991 This PR includes the following changes: ABIRewriteContext: - Added `ArgKind::IndirectAliased` and `ArgClassification::getIndirectAliased` to the MLIR ABI library. CallConvLowering: - Mapped classic `IndirectAliased` to the new kind instead of asserting. CIRABIRewriteContext: - Rewrite a `byref` parameter to a pointer in its own address space. - Attach `llvm.align`, `llvm.noundef` and `llvm.byref(T)` to the parameter and to call operands. - Callee copies the object into its param slot, as for byval. - Call site forwards matching storage or copies into a "byref" slot. - Reject `byref` returns and `va_arg`. Related to issue: https://github.com/llvm/llvm-project/issues/220471 Assisted by : claude opus 5.5 --- Patch is 41.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/230415.diff 10 Files Affected: - (modified) clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp (+9-2) - (modified) clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp (+119-54) - (modified) clang/test/CIR/Transforms/abi-lowering/byval-sret-arg-attr-lowering.cir (+15) - (added) clang/test/CIR/Transforms/abi-lowering/indirect-aliased-nyi.cir (+64) - (added) clang/test/CIR/Transforms/abi-lowering/indirect-aliased.cir (+196) - (modified) mlir/include/mlir/ABI/ABIRewriteContext.h (+16-3) - (modified) mlir/include/mlir/ABI/Targets/Test/TestTarget.h (+4-1) - (modified) mlir/lib/ABI/Targets/Test/TestTarget.cpp (+16-6) - (modified) mlir/unittests/ABI/ABIRewriteContextTest.cpp (+10) - (modified) mlir/unittests/ABI/TestTargetTest.cpp (+48) ``````````diff diff --git a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp index 9f736b920fcc02..a91533c1f582d5 100644 --- a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp +++ b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp @@ -575,6 +575,9 @@ static const llvm::abi::Type *mapCIRType(mlir::Type type, /// wide to fit (a `_BitInt` over 128 bits), is passed via a pointer (sret /// for returns, byval for arguments). /// +/// IndirectAliased: an argument passed as a pointer to the caller's object +/// with no copy implied (byref), in the address space the classifier names. +/// /// Ignore: a void return, or an empty record dropped from the signature. static std::optional<ArgClassification> convertABIArgInfo(const llvm::abi::ArgInfo &info, MLIRContext *ctx, @@ -642,6 +645,9 @@ convertABIArgInfo(const llvm::abi::ArgInfo &info, MLIRContext *ctx, return ArgClassification::getIndirect(info.getIndirectAlign(), info.getIndirectByVal(), info.getIndirectAddrSpace()); + if (info.isIndirectAliased()) + return ArgClassification::getIndirectAliased(info.getIndirectAlign(), + info.getIndirectAddrSpace()); // AArch64 pure scalable aggregates use CoerceAndExpand. This bridge lowers // x86_64 classifications only. assert(!info.isCoerceAndExpand() && @@ -808,12 +814,13 @@ CIRABIClassifierCommon::classifyVariadicCall( } /// Whether \p fc gives the callee access to memory through a pointer the ABI -/// introduced: an sret slot for an indirect return, or an indirect argument. +/// introduced: an sret slot for an indirect return, or an indirect or byref +/// argument. static bool hasIndirectSlot(const FunctionClassification &fc) { if (fc.returnInfo.kind == ArgKind::Indirect) return true; return llvm::any_of(fc.argInfos, [](const ArgClassification &ac) { - return ac.kind == ArgKind::Indirect; + return ac.kind == ArgKind::Indirect || ac.kind == ArgKind::IndirectAliased; }); } diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp index c7ca6151029eb4..970452a60d07f8 100644 --- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp +++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp @@ -28,7 +28,8 @@ using namespace mlir::abi; // This rewrite context supports the Direct (with or without coercion), // Extend, Ignore, Indirect-return (sret), Indirect-argument (byval and -// non-byval), and Expand (struct flattening) classifications. +// non-byval), IndirectAliased-argument (byref), and Expand (struct +// flattening) classifications. // // An Indirect argument is byval or not, following its classification's // byVal flag. byval is a by-value parameter the ABI passes in memory rather @@ -44,6 +45,10 @@ using namespace mlir::abi; // rewires that slot to the incoming pointer so the body mutates the caller's // storage in place. // +// A byref argument is lowered like byval, apart from its attribute and the +// address space of its pointer. The callee must not write through that +// pointer, so it fills the CIRGen param slot with a copy just as byval does. +// // 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 // directly into the parameter's own alloca (the CIRGen spill slot). At the @@ -60,6 +65,22 @@ using namespace mlir::abi; namespace { +/// A pointer to \p pointee in target address space \p addrSpace. Zero is the +/// default address space, which carries no attribute. +cir::PointerType getPtrTyInAddrSpace(mlir::Type pointee, unsigned addrSpace) { + mlir::ptr::MemorySpaceAttrInterface asAttr; + if (addrSpace) + asAttr = cir::TargetAddressSpaceAttr::get(pointee.getContext(), addrSpace); + return cir::PointerType::get(pointee, asAttr); +} + +/// The pointer passed in place of a byref value of type \p pointee. +cir::PointerType getByRefPtrTy(mlir::Type pointee, + const ArgClassification &ac) { + assert(ac.kind == ArgKind::IndirectAliased && "expected a byref argument"); + return getPtrTyInAddrSpace(pointee, ac.indirectAddrSpace); +} + /// Return the coerced RecordType for a Direct classification that should be /// flattened into individual scalar arguments, or a null type if the /// classification does not call for flattening. @@ -82,7 +103,8 @@ cir::RecordType getFlattenedCoercedType(const ArgClassification &ac) { /// Build the new argument-type list for a function whose ABI classification /// is \p fc. Handles Direct (with or without coercion), Extend, Ignore, -/// Indirect (byval and non-byval), and Expand (struct flattening) arguments. +/// Indirect (byval and non-byval), IndirectAliased (byref), and Expand +/// (struct flattening) arguments. /// The sret return pointer, when present, is prepended by /// rewriteFunctionDefinition rather than here. mlir::LogicalResult @@ -135,6 +157,9 @@ buildNewArgTypes(ArrayRef<mlir::Type> oldArgTypes, case ArgKind::Indirect: newArgTypes.push_back(cir::PointerType::get(origTy)); break; + case ArgKind::IndirectAliased: + newArgTypes.push_back(getByRefPtrTy(origTy, ac)); + break; } } return mlir::success(); @@ -142,8 +167,8 @@ buildNewArgTypes(ArrayRef<mlir::Type> oldArgTypes, /// Compute the new return type for a function whose return classification /// is \p retInfo. Direct returns keep (or coerce to) their type, Ignore and -/// Indirect (sret) returns become void, Extend keeps its type; Expand emits -/// an error. +/// Indirect (sret) returns become void, Extend keeps its type; Expand and +/// IndirectAliased emit an error. mlir::Type computeNewReturnType(mlir::Type origRetTy, const ArgClassification &retInfo, mlir::MLIRContext *ctx, @@ -159,6 +184,10 @@ computeNewReturnType(mlir::Type origRetTy, const ArgClassification &retInfo, emitError() << "Expand return is not allowed (classic codegen rejects " << "it in EmitFunctionEpilog)"; return nullptr; + case ArgKind::IndirectAliased: + emitError() << "IndirectAliased return is not allowed (classic codegen " + << "rejects it in EmitFunctionEpilog)"; + return nullptr; case ArgKind::Extend: // Same convention as Extend args: keep the original return type in the // signature; the sign/zero extension is communicated via the @@ -199,25 +228,27 @@ static bool isCoercionWiderThanMemory(mlir::Type origTy, mlir::Type coercedTy, } /// Whether updateArgAttrs has anything to change: an arg that is Ignore (its -/// slot is dropped), Extend (needs llvm.signext / llvm.zeroext), Indirect -/// (needs the pointer attributes), Expand (changes the argument count), or a -/// coerced Direct, which may be flattened or may lose llvm.noundef. +/// slot is dropped), Extend (needs llvm.signext / llvm.zeroext), Indirect or +/// IndirectAliased (needs the pointer attributes), Expand (changes the argument +/// count), or a coerced Direct, which may be flattened or may lose +/// llvm.noundef. static bool needsArgAttrUpdate(const FunctionClassification &fc) { return llvm::any_of(fc.argInfos, [](const ArgClassification &ac) { return ac.kind == ArgKind::Ignore || ac.kind == ArgKind::Extend || - ac.kind == ArgKind::Indirect || ac.kind == ArgKind::Expand || + ac.kind == ArgKind::Indirect || + ac.kind == ArgKind::IndirectAliased || ac.kind == ArgKind::Expand || (ac.kind == ArgKind::Direct && ac.coercedType); }); } /// Build an updated arg_attrs ArrayAttr that drops Ignore'd args, adds /// llvm.signext / llvm.zeroext on Extend args, adds the pointer attributes -/// for Indirect args in place of llvm.nofpclass, drops llvm.noundef from a -/// coerced Direct arg that passes undefined bits, gives each field of a -/// Direct+canFlatten argument the argument's attributes, and gives each field -/// of an Expand argument an empty dict. Direct and Extend slots keep their -/// other existing attributes. \p origArgTypes provides the pre-rewrite type -/// for each arg slot. +/// for Indirect args in place of llvm.nofpclass and for IndirectAliased args, +/// drops llvm.noundef from a coerced Direct arg that passes undefined bits, +/// gives each field of a Direct+canFlatten argument the argument's attributes, +/// and gives each field of an Expand argument an empty dict. Direct and +/// Extend slots keep their other existing attributes. \p origArgTypes +/// provides the pre-rewrite type for each arg slot. mlir::ArrayAttr updateArgAttrs(mlir::MLIRContext *ctx, ArrayRef<mlir::Type> origArgTypes, mlir::ArrayAttr existingArgAttrs, @@ -290,6 +321,16 @@ mlir::ArrayAttr updateArgAttrs(mlir::MLIRContext *ctx, dl.getTypeSize(pointeeTy).getFixedValue())); } newArgAttrs.push_back(attrs.getDictionary(ctx)); + } else if (ac.kind == ArgKind::IndirectAliased) { + // llvm.byref(T) records the pre-rewrite arg type, as llvm.byval does. + mlir::NamedAttrList attrs(existing); + attrs.set(mlir::LLVM::LLVMDialect::getAlignAttrName(), + builder.getI64IntegerAttr(ac.indirectAlign.value())); + attrs.set(mlir::LLVM::LLVMDialect::getNoUndefAttrName(), + builder.getUnitAttr()); + attrs.set(mlir::LLVM::LLVMDialect::getByRefAttrName(), + mlir::TypeAttr::get(origArgTypes[oldIdx])); + newArgAttrs.push_back(attrs.getDictionary(ctx)); } else { newArgAttrs.push_back(existing); } @@ -862,28 +903,28 @@ static cir::LoadOp getWholeRecordLoad(mlir::Value recordVal) { /// Returns \p load if the address it read from can be passed in place of the /// loaded value for an indirect argument that needs \p minAlign alignment, -/// and null otherwise, including when \p load is null. A byval argument -/// also needs storageUnwrittenBetween. On null the caller reports a -/// non-byval argument or fills a byval slot of its own. +/// and null otherwise, including when \p load is null. A byval or byref +/// argument also needs storageUnwrittenBetween. On null the caller reports a +/// non-byval argument or fills a slot of its own. /// -/// The address has to be a pointer to the loaded type in the default address -/// space, since that is what the rewritten parameter is, and a cir.load pins -/// the pointee type without pinning the address space. The storage it names -/// has to be a slot allocated here, reached through storage-preserving casts, -/// already stating at least \p minAlign. The enclosing function's own -/// non-byval parameter qualifies as well, since its slot stands until -/// finalizeParameterSlots and prepareNonByvalParameters has restated it with -/// the alignment that parameter promises rather than the one CIRGen chose -/// for a local copy. A byval parameter's slot keeps CIRGen's alignment, so -/// it qualifies only where that already covers \p minAlign. +/// The address has to have \p paramTy, the rewritten parameter's type, since a +/// cir.load pins the pointee type without pinning the address space. The +/// storage it names has to be a slot allocated here, reached through +/// storage-preserving casts, already stating at least \p minAlign. The +/// enclosing function's own non-byval parameter qualifies as well, since its +/// slot stands until finalizeParameterSlots and prepareNonByvalParameters has +/// restated it with the alignment that parameter promises rather than the one +/// CIRGen chose for a local copy. A byval or byref parameter's slot keeps +/// CIRGen's alignment, so it qualifies only where that already covers +/// \p minAlign. /// /// Raising a slot's alignment here would not hold for one that stands in /// for a parameter, since finalizeParameterSlots replaces it with the /// incoming pointer, which promises only what the caller gave it. static cir::LoadOp forwardableIndirectLoad(cir::LoadOp load, + cir::PointerType paramTy, uint64_t minAlign) { - if (!load || load.getAddr().getType() != - cir::PointerType::get(load.getResult().getType())) + if (!load || load.getAddr().getType() != paramTy) return {}; cir::AllocaOp slot = cir::getUnderlyingAlloca(load.getAddr()); return slot && slot.getAlignment() >= minAlign ? load : cir::LoadOp(); @@ -1004,17 +1045,18 @@ findParamSpill(mlir::BlockArgument blockArg) { /// 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, which find a record in the coercion slot -/// through rewireRecordUses. For each Indirect byval arg, change the block -/// argument's type to a pointer, then replace a record's single param spill -/// with a copy from that pointer. Without such a spill, a record's uses find -/// it at the pointer through rewireRecordUses, and a value of any other type is -/// loaded from the pointer at entry. For each Indirect non-byval arg, change -/// the block argument to a pointer and queue the param-slot alloca to be -/// replaced by it (no entry load or 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. +/// through rewireRecordUses. For each Indirect byval arg and each +/// IndirectAliased arg, change the block argument's type to a pointer, then +/// replace a record's single param spill with a copy from that pointer. +/// Without such a spill, a record's uses find it at the pointer through +/// rewireRecordUses, and a value of any other type is loaded from the pointer +/// at entry. For each Indirect non-byval arg, change the block argument to a +/// pointer and queue the param-slot alloca to be replaced by it (no entry load +/// or 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. /// /// \p hasSRetArg is true when the function has an sret return (a hidden return /// pointer is prepended as block argument 0). Expand arguments expand the @@ -1195,16 +1237,22 @@ insertArgCoercion(mlir::FunctionOpInterface funcOp, blockArg, blockArg, &entry, dl, coercionOps, ac.directOffset))) return mlir::failure(); - } else if (ac.kind == ArgKind::Indirect) { + } else if (ac.kind == ArgKind::Indirect || + ac.kind == ArgKind::IndirectAliased) { // byval and non-byval both lower to !cir.ptr<T>, and which it is shows // up only in the attrs updateArgAttrs applies. Body lowering differs: // the body addresses a byval parameter through the CIRGen slot it was // spilled into, so that slot has to be filled from the incoming // pointer, while a non-byval parameter's slot is replaced by the - // pointer so the body works on the caller's storage in place. - auto ptrTy = cir::PointerType::get(blockArg.getType()); - - if (!ac.byVal) { + // pointer so the body works on the caller's storage in place. A byref + // parameter takes the byval path, through a pointer in its own address + // space. + bool byRef = ac.kind == ArgKind::IndirectAliased; + cir::PointerType ptrTy = byRef + ? getByRefPtrTy(blockArg.getType(), ac) + : cir::PointerType::get(blockArg.getType()); + + if (!byRef && !ac.byVal) { // Without byval, drop the spill store and let the slot's uses read the // incoming pointer, so the body operates on the caller's storage in // place. A byte-copy would be wrong for non-trivially-copyable @@ -2070,8 +2118,15 @@ CIRABIRewriteContext::rewriteCallSite(mlir::Operation *callOp, newArgs.push_back(emitCoercion(builder, call.getLoc(), ac.coercedType, arg, slotBlock, dl, ac.directOffset)); } - } else if (ac.kind == ArgKind::Indirect) { + } else if (ac.kind == ArgKind::Indirect || + ac.kind == ArgKind::IndirectAliased) { uint64_t minAlign = ac.indirectAlign.value(); + // The callee copies a byref object at entry, so the caller treats it + // like byval. + bool byRef = ac.kind == ArgKind::IndirectAliased; + bool byCopy = byRef || ac.byVal; + cir::PointerType ptrTy = byRef ? getByRefPtrTy(arg.getType(), ac) + : cir::PointerType::get(arg.getType()); // A record operand that names storage is passed from that storage // rather than from the loaded value: a record value carries only the @@ -2080,7 +2135,7 @@ CIRABIRewriteContext::rewriteCallSite(mlir::Operation *callOp, // A byval _BitInt is passed from its value, since the load and the // store below are its conversion between value and in-memory form. cir::LoadOp srcLoad; - if (!ac.byVal || mlir::isa<cir::RecordType>(arg.getType())) + if (!byCopy || mlir::isa<cir::RecordType>(arg.getType())) srcLoad = maybeGetSimpleLoad(arg); // A non-byval argument must name the caller's storage, so that the @@ -2088,8 +2143,8 @@ CIRABIRewriteContext::rewriteCallSite(mlir::Operation *callOp, // means passing the address the operand was loaded from rather than the // loaded value, so a store to that storage after the load is visible to // the callee. byval fills a slot below, this cannot. - if (!ac.byVal) { - cir::LoadOp fwdLoad = forwardableIndirectLoad(srcLoad, minAlign); + if (!byCopy) { + cir::LoadOp fwdLoad = forwardableIndirectLoad(srcLoad, ptrTy, minAlign); if (!fwdLoad) return call->emitOpError() << "non-byval indirect argument that does not name the " @@ -2106,7 +2161,8 @@ CIRABIRewriteContext::rewriteCallSite(mlir::Operation *callOp, // call, since the operand carries the value as of the load. A volatile // load is not forwarded, since the copy byval takes at the call is not // volatile. - if (cir::LoadOp fwdLoad = forwardableIndirectLoad(srcLoad, minAlign); + if (cir::LoadOp fwdLoad = + forwardableIndirectLoad(srcLoad, ptrTy, minAlign); fwdLoad && storageUnwrittenBetween(fwdLoad, call.getOperation(), aliasAnalysis)) { newArgs.push_back(fwdLoad.getAddr()); @@ -2114,21 +2170,28 @@ CIRABIRewriteContext::rewriteCallSite(mlir::Operation *callOp, continue; } + // A byref slot has to be an alloca of the parameter's own type, which + // only the alloca address space can hold. + if (byRef && ptrTy != getAllocaPtrTy(arg.getType(), dl)) + return call->emitOpError() + << "byref argument outside the alloca address space that does " + "not name forwardable storage is not yet implemented in " + "CallConvLowering"; + // Otherwise fill a slot, at the load's position when there is one so // the copy reads the memory state the load read. if (failed(rejectAtomicRecordLoad(call, arg))) return mlir::failure(); if (!srcLoad) srcLoad = maybeGetCopyableRecordLoad(arg); - auto ptrTy = cir::PointerType::get(arg.getType()); mlir::IntegerAttr slotAlign = builder.getI64IntegerAttr(minAlign); mlir::OpBuilder::InsertionGuard guard(builder); if (srcLoad) builder.setInsertionPoint(srcLoad); - auto slot = - cir::AllocaOp::c... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/230415 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
