llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Adam Smith (adams381) <details> <summary>Changes</summary> The CallConvLowering bridge accepted only `float` and `double`, so a function taking a `_Complex`, or a float in any other format, failed the pass instead of being classified. An all-float aggregate failed for a different reason: its SSE eightbyte coerces to a vector, and the bridge had no way to represent one, so it reported the coercion NYI rather than emitting a wrong signature. Mapping every CIR floating-point type through `FPTypeInterface` covers all of them at once. A `_Complex` maps to the library's complex type and a vector coercion now converts back to a CIR vector. Accepting a `long double` also makes a union holding one classifiable. That exposes the ABI-compatibility flags, which the pass left at the library defaults. They now come from the triple and the compatibility version, which is what lets a `long double` union reach registers on Darwin instead of memory. `updateArgAttrs` appended argument attributes instead of setting them, so a name already present landed in the dictionary twice. CIRGen marks a `_Complex long double` parameter `llvm.noundef`, and the ABI then passes it byval, which wants `llvm.noundef` too. An integer coercion lost its bit-precise flag coming back from the classifier. A struct holding a `_BitInt(128)` then took `__int128`'s 16-byte alignment for its coerce slot instead of 8. Assisted-by: Cursor / claude-opus-5 --- Patch is 49.68 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/215117.diff 13 Files Affected: - (modified) clang/include/clang/CIR/Dialect/Passes.h (+2-1) - (modified) clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp (+65-34) - (modified) clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp (+15-13) - (modified) clang/lib/CIR/Lowering/CIRPasses.cpp (+31-1) - (added) clang/test/CIR/CodeGen/call-conv-lowering-x86_64-abi-compat.c (+20) - (modified) clang/test/CIR/CodeGen/call-conv-lowering-x86_64.c (+187) - (modified) clang/test/CIR/Transforms/abi-lowering/x86_64-aggregate-nyi.cir (-51) - (modified) clang/test/CIR/Transforms/abi-lowering/x86_64-bitint.cir (+6-6) - (added) clang/test/CIR/Transforms/abi-lowering/x86_64-complex.cir (+75) - (modified) clang/test/CIR/Transforms/abi-lowering/x86_64-variadic-call.cir (+15) - (modified) clang/test/CIR/Transforms/abi-lowering/x86_64-variadic-nyi.cir (-29) - (added) clang/test/CIR/Transforms/abi-lowering/x86_64-vector.cir (+62) - (added) clang/test/CIR/Transforms/abi-lowering/x86_64-wide-floats.cir (+75) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/Passes.h b/clang/include/clang/CIR/Dialect/Passes.h index 0b8142fc394bd..888e7b833b1cf 100644 --- a/clang/include/clang/CIR/Dialect/Passes.h +++ b/clang/include/clang/CIR/Dialect/Passes.h @@ -38,7 +38,8 @@ std::unique_ptr<Pass> createTargetLoweringPass(); std::unique_ptr<Pass> createCallConvLoweringPass(); std::unique_ptr<Pass> createCallConvLoweringPass(cir::CallConvTarget target, - llvm::abi::X86AVXABILevel x86AvxAbiLevel); + llvm::abi::X86AVXABILevel x86AvxAbiLevel, + const llvm::abi::ABICompatInfo &x86AbiCompat); std::unique_ptr<Pass> createHoistAllocasPass(); std::unique_ptr<Pass> createLoweringPreparePass(); std::unique_ptr<Pass> createLoweringPreparePass(clang::ASTContext *astCtx); diff --git a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp index 193c2b6f4a9dc..10ed3d81f8d56 100644 --- a/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp +++ b/clang/lib/CIR/Dialect/Transforms/CallConvLoweringPass.cpp @@ -62,17 +62,16 @@ namespace mlir { namespace { //===----------------------------------------------------------------------===// -// x86_64 System V classifier bridge (scalar and struct/array types) +// x86_64 System V classifier bridge // // Maps CIR types to llvm::abi::Type, runs the LLVM ABI Lowering Library's // SysV x86_64 classifier, and converts the result back into the // dialect-agnostic mlir::abi::FunctionClassification that CIRABIRewriteContext // consumes. Integer (including `_BitInt` up to 128 bits) / pointer / bool / -// f32 / f64 scalars and struct / union / array aggregates are handled. -// `_Complex`, vectors, wider floats, packed or padded records, and a union no -// member of which spans its declared size are reported NYI by -// classifyX86_64Function so an unsupported signature fails the pass instead of -// being misclassified. +// floating-point scalars are handled, as are struct / union / array aggregates +// and `_Complex`. Vectors, packed or padded records, and a union no member of +// which spans its declared size are reported NYI by classifyX86_64Function so +// an unsupported signature fails the pass instead of being misclassified. //===----------------------------------------------------------------------===// /// Whether a struct's declared argument-passing kind (from the module's @@ -101,10 +100,11 @@ static llvm::Align recordDeclaredAlign(ModuleOp modOp, cir::RecordType recTy, } /// The CIR types the x86_64 bridge handles. Scalars: an integer up to 128 -/// bits (including `_BitInt` and `__int128`), pointer, bool, void, f32, or f64. -/// Aggregates: a complete struct or union whose members are all themselves -/// supported, or an array of a supported element type. Everything else is -/// reported NYI at the reject() choke point in classifyX86_64Function. +/// bits (including `_BitInt` and `__int128`), pointer, bool, void, or any +/// floating-point type. Aggregates: a complete struct or union whose members +/// are all themselves supported, or an array of a supported element type. +/// Also a `_Complex` of a supported element type. Everything else is reported +/// NYI at the reject() choke point in classifyX86_64Function. static bool isSupportedType(mlir::Type ty, const DataLayout &dl) { // A pointer is only handled in the default address space (null) or an // already-lowered target address space. A LangAddressSpaceAttr must be @@ -112,7 +112,11 @@ static bool isSupportedType(mlir::Type ty, const DataLayout &dl) { if (auto ptrTy = dyn_cast<cir::PointerType>(ty)) return !ptrTy.getAddrSpace() || mlir::isa<cir::TargetAddressSpaceAttr>(ptrTy.getAddrSpace()); - if (isa<cir::VoidType, cir::BoolType, cir::SingleType, cir::DoubleType>(ty)) + if (isa<cir::VoidType, cir::BoolType>(ty)) + return true; + // Every CIR floating-point type carries the semantics the classifier + // switches on, so all of them are handled. + if (isa<cir::FPTypeInterface>(ty)) return true; if (auto intTy = dyn_cast<cir::IntType>(ty)) { // Integers up to 64 bits, __int128, and _BitInt up to 128 bits are @@ -129,6 +133,8 @@ static bool isSupportedType(mlir::Type ty, const DataLayout &dl) { return intTy.getWidth() <= 128; return intTy.getWidth() <= 64 || intTy.getWidth() == 128; } + if (auto complexTy = dyn_cast<cir::ComplexType>(ty)) + return isSupportedType(complexTy.getElementType(), dl); if (auto arrTy = dyn_cast<cir::ArrayType>(ty)) return isSupportedType(arrTy.getElementType(), dl); if (auto recTy = dyn_cast<cir::RecordType>(ty)) { @@ -177,7 +183,7 @@ static mlir::Type abiTypeToCIR(const llvm::abi::Type *ty, MLIRContext *ctx) { [&](const llvm::abi::VoidType *) { return cir::VoidType::get(ctx); }) .Case([&](const llvm::abi::IntegerType *intTy) { return cir::IntType::get(ctx, intTy->getSizeInBits().getFixedValue(), - intTy->isSigned()); + intTy->isSigned(), intTy->isBitInt()); }) .Case([&](const llvm::abi::FloatType *fltTy) { return cir::getFloatingPointType(*fltTy->getSemantics(), ctx); @@ -185,6 +191,13 @@ static mlir::Type abiTypeToCIR(const llvm::abi::Type *ty, MLIRContext *ctx) { .Case([&](const llvm::abi::PointerType *) { return cir::PointerType::get(cir::VoidType::get(ctx)); }) + .Case([&](const llvm::abi::VectorType *vecTy) -> mlir::Type { + mlir::Type elemCIR = abiTypeToCIR(vecTy->getElementType(), ctx); + if (!elemCIR) + return nullptr; + return cir::VectorType::get(elemCIR, + vecTy->getNumElements().getFixedValue()); + }) .Case([&](const llvm::abi::RecordType *recTy) -> mlir::Type { SmallVector<mlir::Type> fieldTypes; fieldTypes.reserve(recTy->getFields().size()); @@ -230,13 +243,16 @@ static const llvm::abi::Type *mapCIRType(mlir::Type type, /*Signed=*/false); }) .Case([&](cir::VoidType) { return tb.getVoidType(); }) - .Case([&](cir::SingleType) { - return tb.getFloatType(llvm::APFloat::IEEEsingle(), + .Case([&](cir::FPTypeInterface fpTy) { + // LongDoubleType reports its underlying format's semantics, so the + // classifier sees x87 or IEEE quad rather than the wrapper. + return tb.getFloatType(fpTy.getFloatSemantics(), llvm::Align(dl.getTypeABIAlignment(type))); }) - .Case([&](cir::DoubleType) { - return tb.getFloatType(llvm::APFloat::IEEEdouble(), - llvm::Align(dl.getTypeABIAlignment(type))); + .Case([&](cir::ComplexType complexTy) { + return tb.getComplexType( + mapCIRType(complexTy.getElementType(), typeMapper, dl, modOp), + llvm::Align(dl.getTypeABIAlignment(type))); }) .Case([&](cir::ArrayType arrTy) { const llvm::abi::Type *elemAbi = @@ -296,8 +312,8 @@ static const llvm::abi::Type *mapCIRType(mlir::Type type, /// eightbyte. getDirect keeps canFlatten set so the rewriter can split a /// multi-field coerced struct into individual wire arguments. Any other scalar /// passes in its natural CIR type, which a null coercion denotes. A coercion -/// this bridge cannot represent (an SSE <2 x float>, say) yields std::nullopt -/// so the caller reports NYI rather than silently passing the value unchanged. +/// this bridge cannot represent yields std::nullopt so the caller reports NYI +/// rather than silently passing the value unchanged. /// /// Extend: bool or a sub-register integer needs a signext/zeroext attribute. /// The x86_64 classifier (llvm/lib/ABI/Targets/X86.cpp) only returns Extend @@ -314,12 +330,12 @@ convertABIArgInfo(const llvm::abi::ArgInfo &info, MLIRContext *ctx, if (info.isDirect()) { // The classifier names a coerce type even where it matches the natural // type, so a non-null coerce does not by itself mean a rewrite is needed. - // Leaving a scalar alone also preserves its ABI alignment: abiTypeToCIR - // drops the bit-precise flag, so a _BitInt(128) routed through it would - // come back as !cir.int<s, 128> with __int128's 16-byte alignment instead - // of 8. const llvm::abi::Type *coerceAbi = info.getCoerceToType(); bool isAggregate = isa_and_present<cir::RecordType, cir::ArrayType>(origTy); + // For a _Complex the classifier's coerce is only sometimes the natural + // type, so it has to be read rather than assumed. + bool comparesAgainstCoerce = + coerceAbi && isa_and_present<cir::ComplexType>(origTy); bool coerceIsRegisterTuple = isa_and_present<llvm::abi::RecordType>(coerceAbi); // Compare widths rather than identity: a coerce no wider than the natural @@ -330,15 +346,19 @@ convertABIArgInfo(const llvm::abi::ArgInfo &info, MLIRContext *ctx, bool coerceWidensScalar = origInt && coerceInt && coerceInt->getSizeInBits().getFixedValue() > origInt.getWidth(); - if (!isAggregate && !coerceIsRegisterTuple && !coerceWidensScalar) + // Leaving the rest alone also avoids a lossy round trip: abiTypeToCIR + // drops the LongDoubleType wrapper and a pointer's pointee, so comparing a + // scalar against its own coerce would report a difference that is not one. + if (!isAggregate && !comparesAgainstCoerce && !coerceIsRegisterTuple && + !coerceWidensScalar) return ArgClassification::getDirect(nullptr); - // The coerce must be a type this bridge can represent. One it cannot map - // (an SSE vector, or a nested type it does not handle) yields a null type. - // Report that as NYI instead of leaving the value as an unchanged by-value - // record. mlir::Type coerced = abiTypeToCIR(coerceAbi, ctx); if (!coerced) return std::nullopt; + // Coercing a value to the type it already has would add a memory round + // trip for nothing. + if (comparesAgainstCoerce && coerced == origTy) + return ArgClassification::getDirect(nullptr); return ArgClassification::getDirect(coerced); } if (info.isExtend()) { @@ -412,9 +432,8 @@ static std::optional<FunctionClassification> classifyX86_64Signature( llvm::CallingConv::C, retAbi, argAbi, required); targetInfo.computeInfo(*fi); - // convertABIArgInfo returns nullopt when the classifier picks a coercion - // this bridge cannot represent (e.g. an SSE vector coerce for an all-float - // aggregate). Report it as NYI rather than emitting a wrong signature. + // convertABIArgInfo returns nullopt when the classifier picks a coercion this + // bridge cannot represent. auto nyiCoercion = [&](mlir::Type t) { emitError() << "x86_64 calling-convention lowering not yet " "implemented for the ABI coercion of type " @@ -499,7 +518,18 @@ static bool classifiesSamePrefix(const FunctionClassification &calleeFc, struct CallConvLoweringPass : public impl::CallConvLoweringBase<CallConvLoweringPass> { using CallConvLoweringBase::CallConvLoweringBase; + + CallConvLoweringPass(const CallConvLoweringOptions &options, + const llvm::abi::ABICompatInfo &x86AbiCompat) + : CallConvLoweringBase(options), x86AbiCompat(x86AbiCompat) {} + void runOnOperation() override; + + /// The x86_64 flags whose value depends on the target and the requested ABI + /// compatibility version. Carried outside the pass options because the + /// struct has no command-line parser, so a cir-opt run gets the library + /// defaults rather than a target's values. + llvm::abi::ABICompatInfo x86AbiCompat; }; /// Record on \p fc whether \p returnType is CIR's void. The x86_64 classifier @@ -610,7 +640,7 @@ void CallConvLoweringPass::runOnOperation() { x86TypeMapper.emplace(dl); x86Target = llvm::abi::createX86_64TargetInfo( x86TypeMapper->getTypeBuilder(), x86AvxAbiLevel.getValue(), - /*Has64BitPointers=*/true, llvm::abi::ABICompatInfo()); + /*Has64BitPointers=*/true, x86AbiCompat); } // Classify every cir.func up front. No IR mutation happens here, so @@ -827,9 +857,10 @@ std::unique_ptr<Pass> mlir::createCallConvLoweringPass() { std::unique_ptr<Pass> mlir::createCallConvLoweringPass(cir::CallConvTarget target, - llvm::abi::X86AVXABILevel x86AvxAbiLevel) { + llvm::abi::X86AVXABILevel x86AvxAbiLevel, + const llvm::abi::ABICompatInfo &x86AbiCompat) { CallConvLoweringOptions options; options.target = target; options.x86AvxAbiLevel = x86AvxAbiLevel; - return std::make_unique<CallConvLoweringPass>(options); + return std::make_unique<CallConvLoweringPass>(options, x86AbiCompat); } diff --git a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp index a8b7f60b6a014..7c80aa300d642 100644 --- a/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp +++ b/clang/lib/CIR/Dialect/Transforms/TargetLowering/CIRABIRewriteContext.cpp @@ -179,6 +179,11 @@ mlir::Value createIgnoredValue(mlir::OpBuilder &builder, mlir::Location loc, /// llvm.align on Indirect args. Preserves any existing arg attributes on /// retained arg slots. \p origArgTypes provides the pre-rewrite type for /// each arg slot (needed to compute the llvm.byval pointee type). +/// +/// An attribute this function sets can already be present on the arg slot: +/// CIRGen marks a scalar parameter llvm.noundef, and the ABI can then pass that +/// parameter byval, which wants llvm.noundef too. So each name has to be set +/// rather than appended, or the dictionary carries it twice. mlir::ArrayAttr updateArgAttrs(mlir::MLIRContext *ctx, ArrayRef<mlir::Type> origArgTypes, mlir::ArrayAttr existingArgAttrs, @@ -204,9 +209,9 @@ mlir::ArrayAttr updateArgAttrs(mlir::MLIRContext *ctx, newArgAttrs.append(recTy.getNumElements(), builder.getDictionaryAttr({})); } else if (ac.kind == ArgKind::Extend) { StringRef attrName = ac.signExtend ? "llvm.signext" : "llvm.zeroext"; - SmallVector<mlir::NamedAttribute> attrs(existing.begin(), existing.end()); - attrs.push_back(builder.getNamedAttr(attrName, builder.getUnitAttr())); - newArgAttrs.push_back(builder.getDictionaryAttr(attrs)); + mlir::NamedAttrList attrs(existing); + attrs.set(attrName, builder.getUnitAttr()); + newArgAttrs.push_back(attrs.getDictionary(ctx)); } else if (ac.kind == ArgKind::Indirect) { // byval: caller-allocated copy; callee receives pointer to copy. // byref: callee receives pointer to the caller's original storage. @@ -226,18 +231,15 @@ mlir::ArrayAttr updateArgAttrs(mlir::MLIRContext *ctx, // produces a fresh alloca+store. mlir::Type pointeeTy = origArgTypes[oldIdx]; StringRef ownershipAttr = ac.byVal ? "llvm.byval" : "llvm.byref"; - SmallVector<mlir::NamedAttribute> attrs(existing.begin(), existing.end()); - attrs.push_back(builder.getNamedAttr( - "llvm.align", builder.getI64IntegerAttr(ac.indirectAlign.value()))); - attrs.push_back( - builder.getNamedAttr(ownershipAttr, mlir::TypeAttr::get(pointeeTy))); + mlir::NamedAttrList attrs(existing); + attrs.set("llvm.align", + builder.getI64IntegerAttr(ac.indirectAlign.value())); + attrs.set(ownershipAttr, mlir::TypeAttr::get(pointeeTy)); if (ac.byVal) { - attrs.push_back( - builder.getNamedAttr("llvm.noalias", builder.getUnitAttr())); - attrs.push_back( - builder.getNamedAttr("llvm.noundef", builder.getUnitAttr())); + attrs.set("llvm.noalias", builder.getUnitAttr()); + attrs.set("llvm.noundef", builder.getUnitAttr()); } - newArgAttrs.push_back(builder.getDictionaryAttr(attrs)); + newArgAttrs.push_back(attrs.getDictionary(ctx)); } else { newArgAttrs.push_back(existing); } diff --git a/clang/lib/CIR/Lowering/CIRPasses.cpp b/clang/lib/CIR/Lowering/CIRPasses.cpp index 0a683b8c8a498..dad17f2ef8659 100644 --- a/clang/lib/CIR/Lowering/CIRPasses.cpp +++ b/clang/lib/CIR/Lowering/CIRPasses.cpp @@ -13,6 +13,7 @@ #include "mlir/IR/BuiltinOps.h" #include "mlir/Pass/PassManager.h" #include "clang/AST/ASTContext.h" +#include "clang/Basic/LangOptions.h" #include "clang/Basic/TargetInfo.h" #include "clang/CIR/Dialect/Passes.h" #include "llvm/Support/TimeProfiler.h" @@ -28,6 +29,34 @@ static CallConvTarget getCallConvTarget(const llvm::Triple &triple) { return CallConvTarget::None; } +/// The x86_64 ABI-compatibility flags, derived from the target and the +/// requested compatibility version. Every flag defaults to true in the ABI +/// library, which is not what any target computes: Clang11Compat is false for a +/// modern Linux target, so leaving it at the default classifies a union larger +/// than an eightbyte as though every member spanned its size. Mirrors the +/// predicates in clang/lib/CodeGen/Targets/X86.cpp and the derivation in +/// CodeGenModule::getLLVMABITargetInfo, which computes the same five flags for +/// the classic path. +static llvm::abi::ABICompatInfo +getX86ABICompatInfo(const clang::ASTContext &astContext) { + const llvm::Triple &triple = astContext.getTargetInfo().getTriple(); + const clang::LangOptions &langOpts = astContext.getLangOpts(); + clang::LangOptions::ClangABI compat = langOpts.getClangABICompat(); + llvm::abi::ABICompatInfo abiCompat; + abiCompat.HonorsRevision98 = !triple.isOSDarwin(); + abiCompat.ClassifyIntegerMMXAsSSE = + compat > clang::LangOptions::ClangABI::Ver3_8 && !triple.isOSDarwin() && + !triple.isPS() && !triple.isOSFreeBSD(); + abiCompat.PassInt128VectorsInMem = + compat > clang::LangOptions::ClangABI::Ver9 && + (triple.isOSLinux() || triple.isOSNetBSD()); + abiCompat.ReturnCXXRecordGreaterThan128InMem = + compat > clang::LangOptions::ClangABI::Ver20 && !triple.isPS(); + abiCompat.Clang11Compat = + compat <= clang::LangOptions::ClangABI::Ver11 || triple.isPS(); + return abiCompat; +} + mlir::LogicalResult runCIRToCIRPasses(mlir::ModuleOp theModule, mlir::MLIRContext &mlirContext, clang::ASTContext &astContext, bool enableVerifier, @@ -70,7 +99,8 @@ runCIRToCIRPasses(mlir::ModuleOp theModule, mlir::MLIRContext &mlirContext, getCallConvTarget(astContext.getTargetInfo().getTriple()); if (target != CallConvTarget::None) pm.addPass(mlir::createCallConvLoweringPass( - target, llvm::abi::X86AVXABILevel::None)); + target, llvm::abi::X86AVXABILevel::None, + getX86ABICompatInfo(astContext))); } pm.addPass(mlir::createLoweringPreparePass(&astContext)); diff --git a/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-abi-compat.c b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-abi-compat.c new file mode 100644 index 0000000000000..6a1d9a3c4d3d2 --- /dev/null +++ b/clang/test/CIR/CodeGen/call-conv-lowering-x86_64-abi-compat.c @@ -0,0 +1,20 @@ +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -clangir-enable-call-conv-lowering -emit-llvm %s -o %t-cir.ll +// RUN: FileCheck --check-prefix=LINUX-CIR --input-file=%t-cir.ll %s +// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o %t.ll +// RUN: FileCheck --check-prefix=LINUX-OGCG --input-file=%t.ll %s + +// RUN: %clang_cc1 -triple x86_64-apple-darwin -fclangir -clangir-enable-call-conv-lowering -emit-llvm %s -o %t-darwin-cir.ll +// RUN: FileCheck --check-prefix=DARWIN --input-file=%t-darwin-cir.ll %s +// RUN: %clang_cc1 -triple x86_64-apple-darwin -emit-llvm %s -o %t-darwin.ll +// RUN: FileCheck --check-prefix=DARWIN --input-file=%t-darwin.ll %s + +// The 0.98 ABI revision sends an eightbyte pair to memory when the high half is +// X87UP and the low half is not X87. Darwin exempts itself for binary +// compatibility with older GCC, so the same union passes in registers there. +// The int member is what makes the low half INTEGER rather than X87. +typedef union { long double l; int i; } ULongDouble; +void rev98(ULongDouble ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/215117 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
