llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Erich Keane (erichkeane) <details> <summary>Changes</summary> This came up during self-build, we are spending a lot of time in some cases looking up using the global symbol table, which does no caching. Previously we'd propagated this in a few places, but this patch removes all uses of SymbolTable::lookup and adds the cache everywhere. This involved changing the tablegen to include it in each of our rewriters, plus the CirAttr lowering everywhere. The only thing we have to take care of is to make sure we invalidate the cache/update the cache whenever we add something with a name (see createLLVMFuncOpIfNotExist). This is NFC, as it isn't observable, other than being a build time improvement. --- Patch is 24.85 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/213972.diff 6 Files Affected: - (modified) clang/include/clang/CIR/Dialect/IR/CIROps.td (-6) - (modified) clang/include/clang/CIR/LoweringHelpers.h (+6-8) - (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+47-34) - (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h (+1) - (modified) clang/lib/CIR/Lowering/LoweringHelpers.cpp (+27-29) - (modified) clang/utils/TableGen/CIRLoweringEmitter.cpp (+5-2) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/IR/CIROps.td b/clang/include/clang/CIR/Dialect/IR/CIROps.td index ba8011e03f638..4c8490e528154 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIROps.td +++ b/clang/include/clang/CIR/Dialect/IR/CIROps.td @@ -4579,9 +4579,6 @@ def CIR_CallOp : CIR_CallOpBase<"call", [NoRegionArguments]> { $_state.addTypes(resType); }]> ]; - - let customLLVMLoweringConstructorDecl = - LoweringBuilders<(ins "mlir::SymbolTableCollection &":$symbolTables)>; } def CIR_TryCallOp : CIR_CallOpBase<"try_call",[ @@ -4671,9 +4668,6 @@ def CIR_TryCallOp : CIR_CallOpBase<"try_call",[ $_state.addSuccessors(unwindDest); }]> ]; - - let customLLVMLoweringConstructorDecl = - LoweringBuilders<(ins "mlir::SymbolTableCollection &":$symbolTables)>; } //===----------------------------------------------------------------------===// diff --git a/clang/include/clang/CIR/LoweringHelpers.h b/clang/include/clang/CIR/LoweringHelpers.h index b83add4c732a2..92633c89e1369 100644 --- a/clang/include/clang/CIR/LoweringHelpers.h +++ b/clang/include/clang/CIR/LoweringHelpers.h @@ -34,15 +34,13 @@ convertToDenseElementsAttr(cir::ConstArrayAttr attr, const llvm::SmallVectorImpl<int64_t> &dims, mlir::Type type); -std::optional<mlir::Attribute> -lowerConstArrayAttr(cir::ConstArrayAttr constArr, - const mlir::TypeConverter *converter, - mlir::ModuleOp moduleOp = {}); +std::optional<mlir::Attribute> lowerConstArrayAttr( + cir::ConstArrayAttr constArr, mlir::SymbolTableCollection &symbolTables, + const mlir::TypeConverter *converter, mlir::ModuleOp moduleOp = {}); -std::optional<mlir::Attribute> -lowerConstRecordAttr(cir::ConstRecordAttr constRecord, - const mlir::TypeConverter *converter, - mlir::ModuleOp moduleOp = {}); +std::optional<mlir::Attribute> lowerConstRecordAttr( + cir::ConstRecordAttr constRecord, mlir::SymbolTableCollection &symbolTables, + const mlir::TypeConverter *converter, mlir::ModuleOp moduleOp = {}); /// Adjust \p llvmType (the converted type of \p init) to the concrete LLVM type /// a global constant initialized with \p init actually lowers to. This differs diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp index 70fe6e22e77bc..40e4abc3b95c3 100644 --- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp +++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp @@ -384,10 +384,11 @@ class CIRAttrToValue { public: CIRAttrToValue(mlir::Operation *parentOp, mlir::ConversionPatternRewriter &rewriter, + mlir::SymbolTableCollection &symbolTables, const mlir::TypeConverter *converter, LLVMBlockAddressInfo *blockInfoAddr = nullptr) - : parentOp(parentOp), rewriter(rewriter), converter(converter), - blockInfoAddr(blockInfoAddr) {} + : parentOp(parentOp), rewriter(rewriter), symbolTables(symbolTables), + converter(converter), blockInfoAddr(blockInfoAddr) {} #define GET_CIR_ATTR_TO_VALUE_VISITOR_DECLS #include "clang/CIR/Dialect/IR/CIRLowering.inc" @@ -396,6 +397,7 @@ class CIRAttrToValue { private: mlir::Operation *parentOp; mlir::ConversionPatternRewriter &rewriter; + mlir::SymbolTableCollection &symbolTables; const mlir::TypeConverter *converter; // Only available when lowering global initializers that may contain block // address attributes. Used to resolve a BlockAddrInfoAttr to its block tag. @@ -406,9 +408,11 @@ class CIRAttrToValue { mlir::Value lowerCirAttrAsValue(mlir::Operation *parentOp, const mlir::Attribute attr, mlir::ConversionPatternRewriter &rewriter, + mlir::SymbolTableCollection &symbolTables, const mlir::TypeConverter *converter, LLVMBlockAddressInfo *blockInfoAddr) { - CIRAttrToValue valueConverter(parentOp, rewriter, converter, blockInfoAddr); + CIRAttrToValue valueConverter(parentOp, rewriter, symbolTables, converter, + blockInfoAddr); mlir::Value value = valueConverter.visit(attr); if (!value) llvm_unreachable("unhandled attribute type"); @@ -700,7 +704,7 @@ mlir::Value CIRAttrToValue::visitCirAttr(cir::ConstArrayAttr attr) { // When the array can be represented as a single dense constant, emit one // llvm.mlir.constant instead of a chain of llvm.insertvalue ops. if (std::optional<mlir::Attribute> denseAttr = - lowerConstArrayAttr(attr, converter)) + lowerConstArrayAttr(attr, symbolTables, converter)) return mlir::LLVM::ConstantOp::create(rewriter, loc, llvmTy, *denseAttr); if (attr.hasTrailingZeros()) { @@ -790,7 +794,7 @@ mlir::Value CIRAttrToValue::visitCirAttr(cir::GlobalViewAttr globalAttr) { unsigned sourceAddrSpace = 0; llvm::StringRef symName; mlir::Operation *sourceSymbol = - mlir::SymbolTable::lookupSymbolIn(moduleOp, globalAttr.getSymbol()); + symbolTables.lookupSymbolIn(moduleOp, globalAttr.getSymbol()); if (auto llvmSymbol = dyn_cast<mlir::LLVM::GlobalOp>(sourceSymbol)) { sourceType = llvmSymbol.getType(); symName = llvmSymbol.getSymName(); @@ -2376,7 +2380,8 @@ mlir::LogicalResult CIRToLLVMConstantOpLowering::matchAndRewrite( } // Lower GlobalViewAttr to llvm.mlir.addressof if (auto gv = mlir::dyn_cast<cir::GlobalViewAttr>(op.getValue())) { - auto newOp = lowerCirAttrAsValue(op, gv, rewriter, getTypeConverter()); + auto newOp = lowerCirAttrAsValue(op, gv, rewriter, symbolTables, + getTypeConverter()); rewriter.replaceOp(op, newOp); return mlir::success(); } @@ -2387,28 +2392,30 @@ mlir::LogicalResult CIRToLLVMConstantOpLowering::matchAndRewrite( return op.emitError() << "array does not have a constant initializer"; std::optional<mlir::Attribute> denseAttr; - if (constArr && - (denseAttr = lowerConstArrayAttr(constArr, typeConverter))) { + if (constArr && (denseAttr = lowerConstArrayAttr(constArr, symbolTables, + typeConverter))) { attr = denseAttr.value(); } else { - const mlir::Value initVal = - lowerCirAttrAsValue(op, op.getValue(), rewriter, typeConverter); + const mlir::Value initVal = lowerCirAttrAsValue( + op, op.getValue(), rewriter, symbolTables, typeConverter); rewriter.replaceOp(op, initVal); return mlir::success(); } } else if (const auto recordAttr = mlir::dyn_cast<cir::ConstRecordAttr>(op.getValue())) { - auto initVal = lowerCirAttrAsValue(op, recordAttr, rewriter, typeConverter); + auto initVal = lowerCirAttrAsValue(op, recordAttr, rewriter, symbolTables, + typeConverter); rewriter.replaceOp(op, initVal); return mlir::success(); } else if (const auto vecTy = mlir::dyn_cast<cir::VectorType>(op.getType())) { - rewriter.replaceOp(op, lowerCirAttrAsValue(op, op.getValue(), rewriter, - getTypeConverter())); + rewriter.replaceOp(op, + lowerCirAttrAsValue(op, op.getValue(), rewriter, + symbolTables, getTypeConverter())); return mlir::success(); } else if (mlir::isa<cir::RecordType>(op.getType())) { if (mlir::isa<cir::ZeroAttr, cir::UndefAttr>(attr)) { mlir::Value initVal = - lowerCirAttrAsValue(op, attr, rewriter, typeConverter); + lowerCirAttrAsValue(op, attr, rewriter, symbolTables, typeConverter); rewriter.replaceOp(op, initVal); return mlir::success(); } @@ -2821,7 +2828,8 @@ CIRToLLVMGlobalOpLowering::matchAndRewriteRegionInitializedGlobal( // Pass blockInfoAddr so that block address initializers (either as the whole // initializer or nested inside an aggregate) can be resolved by the // BlockAddrInfoAttr visitor. - CIRAttrToValue valueConverter(op, rewriter, typeConverter, &blockInfoAddr); + CIRAttrToValue valueConverter(op, rewriter, symbolTables, typeConverter, + &blockInfoAddr); mlir::Value value = valueConverter.visit(init); mlir::LLVM::ReturnOp::create(rewriter, loc, value); return mlir::success(); @@ -2909,8 +2917,8 @@ mlir::LogicalResult CIRToLLVMGlobalOpLowering::matchAndRewrite( if (isBulkLowerableConstArrayBaseElement( getConstArrayBaseElementType(constArr.getType()))) { mlir::ModuleOp modOp = op->getParentOfType<mlir::ModuleOp>(); - if (std::optional<mlir::Attribute> bulkInit = - lowerConstArrayAttr(constArr, typeConverter, modOp)) { + if (std::optional<mlir::Attribute> bulkInit = lowerConstArrayAttr( + constArr, symbolTables, typeConverter, modOp)) { mlir::SymbolRefAttr comdatAttr = getComdatAttr(op, rewriter); rewriter.replaceOpWithNewOp<mlir::LLVM::GlobalOp>( op, llvmType, isConst, linkage, symbol, bulkInit.value(), @@ -2928,8 +2936,8 @@ mlir::LogicalResult CIRToLLVMGlobalOpLowering::matchAndRewrite( // llvm::ConstantStruct, so the whole initializer becomes a single // attribute on the global instead of an insertvalue region. mlir::ModuleOp modOp = op->getParentOfType<mlir::ModuleOp>(); - if (std::optional<mlir::Attribute> bulkInit = - lowerConstRecordAttr(constRecord, typeConverter, modOp)) { + if (std::optional<mlir::Attribute> bulkInit = lowerConstRecordAttr( + constRecord, symbolTables, typeConverter, modOp)) { mlir::SymbolRefAttr comdatAttr = getComdatAttr(op, rewriter); rewriter.replaceOpWithNewOp<mlir::LLVM::GlobalOp>( op, llvmType, isConst, linkage, symbol, bulkInit.value(), alignment, @@ -4196,15 +4204,13 @@ void ConvertCIRToLLVMPass::runOnOperation() { mlir::RewritePatternSet patterns(&getContext()); patterns.add<CIRToLLVMBlockAddressOpLowering, CIRToLLVMGlobalOpLowering, CIRToLLVMLabelOpLowering>(converter, patterns.getContext(), dl, - blockInfoAddr); - patterns.add<CIRToLLVMCallOpLowering, CIRToLLVMTryCallOpLowering>( - converter, patterns.getContext(), dl, symbolTables); + symbolTables, blockInfoAddr); patterns.add< #define GET_LLVM_LOWERING_PATTERNS_LIST #include "clang/CIR/Dialect/IR/CIRLowering.inc" #undef GET_LLVM_LOWERING_PATTERNS_LIST - >(converter, patterns.getContext(), dl); + >(converter, patterns.getContext(), dl, symbolTables); processCIRAttrs(module); @@ -4332,13 +4338,14 @@ mlir::LogicalResult CIRToLLVMInsertMemberOpLowering::matchAndRewrite( } void createLLVMFuncOpIfNotExist(mlir::ConversionPatternRewriter &rewriter, + mlir::SymbolTableCollection &symbolTables, mlir::Operation *srcOp, llvm::StringRef fnName, mlir::Type fnTy, mlir::ArrayAttr argAttrs = nullptr, mlir::ArrayAttr resAttrs = nullptr) { mlir::ModuleOp modOp = srcOp->getParentOfType<mlir::ModuleOp>(); - mlir::Operation *sourceSymbol = - mlir::SymbolTable::lookupSymbolIn(modOp, fnName); + mlir::Operation *sourceSymbol = symbolTables.lookupSymbolIn( + modOp, mlir::StringAttr::get(fnTy.getContext(), fnName)); if (!sourceSymbol) { mlir::OpBuilder::InsertionGuard guard(rewriter); auto enclosingFnOp = srcOp->getParentOfType<mlir::LLVM::LLVMFuncOp>(); @@ -4349,6 +4356,8 @@ void createLLVMFuncOpIfNotExist(mlir::ConversionPatternRewriter &rewriter, fn.setArgAttrsAttr(argAttrs); if (resAttrs) fn.setResAttrsAttr(resAttrs); + // Update the symbol table. + symbolTables.getSymbolTable(fn->getParentOp()).insert(fn); } } @@ -4363,7 +4372,8 @@ mlir::LogicalResult CIRToLLVMThrowOpLowering::matchAndRewrite( // Get or create `declare void @__cxa_rethrow()` const llvm::StringRef functionName = "__cxa_rethrow"; - createLLVMFuncOpIfNotExist(rewriter, op, functionName, funcTy); + createLLVMFuncOpIfNotExist(rewriter, symbolTables, op, functionName, + funcTy); auto cxaRethrow = mlir::LLVM::CallOp::create( rewriter, loc, mlir::TypeRange{}, functionName); @@ -4378,7 +4388,7 @@ mlir::LogicalResult CIRToLLVMThrowOpLowering::matchAndRewrite( // Get or create `declare void @__cxa_throw(ptr, ptr, ptr)` const llvm::StringRef fnName = "__cxa_throw"; - createLLVMFuncOpIfNotExist(rewriter, op, fnName, fnTy); + createLLVMFuncOpIfNotExist(rewriter, symbolTables, op, fnName, fnTy); mlir::Value typeInfo = mlir::LLVM::AddressOfOp::create( rewriter, loc, mlir::LLVM::LLVMPointerType::get(rewriter.getContext()), @@ -4409,7 +4419,7 @@ mlir::LogicalResult CIRToLLVMAllocExceptionOpLowering::matchAndRewrite( auto int64Ty = mlir::IntegerType::get(rewriter.getContext(), 64); auto fnTy = mlir::LLVM::LLVMFunctionType::get(llvmPtrTy, {int64Ty}); - createLLVMFuncOpIfNotExist(rewriter, op, fnName, fnTy); + createLLVMFuncOpIfNotExist(rewriter, symbolTables, op, fnName, fnTy); auto exceptionSize = mlir::LLVM::ConstantOp::create(rewriter, op.getLoc(), adaptor.getSizeAttr()); @@ -4566,10 +4576,11 @@ mlir::LogicalResult CIRToLLVMTrapOpLowering::matchAndRewrite( static mlir::Value getValueForVTableSymbol(mlir::Operation *op, mlir::ConversionPatternRewriter &rewriter, + mlir::SymbolTableCollection &symbolTables, const mlir::TypeConverter *converter, mlir::FlatSymbolRefAttr nameAttr, mlir::Type &eltType) { auto module = op->getParentOfType<mlir::ModuleOp>(); - mlir::Operation *symbol = mlir::SymbolTable::lookupSymbolIn(module, nameAttr); + mlir::Operation *symbol = symbolTables.lookupSymbolIn(module, nameAttr); if (auto llvmSymbol = mlir::dyn_cast<mlir::LLVM::GlobalOp>(symbol)) { eltType = llvmSymbol.getType(); } else if (auto cirSymbol = mlir::dyn_cast<cir::GlobalOp>(symbol)) { @@ -4591,8 +4602,8 @@ mlir::LogicalResult CIRToLLVMVTableAddrPointOpLowering::matchAndRewrite( mlir::Type targetType = converter->convertType(op.getType()); llvm::SmallVector<mlir::LLVM::GEPArg> offsets; mlir::Type eltType; - mlir::Value symAddr = getValueForVTableSymbol(op, rewriter, converter, - op.getNameAttr(), eltType); + mlir::Value symAddr = getValueForVTableSymbol( + op, rewriter, symbolTables, converter, op.getNameAttr(), eltType); if (!symAddr) return op.emitError() << "Unable to get value for vtable symbol"; @@ -4649,8 +4660,9 @@ mlir::LogicalResult CIRToLLVMVTTAddrPointOpLowering::matchAndRewrite( offsets.push_back(adaptor.getOffset()); eltType = mlir::LLVM::LLVMPointerType::get(rewriter.getContext()); } else { - llvmAddr = getValueForVTableSymbol(op, rewriter, getTypeConverter(), - op.getNameAttr(), eltType); + llvmAddr = + getValueForVTableSymbol(op, rewriter, symbolTables, getTypeConverter(), + op.getNameAttr(), eltType); assert(eltType && "Shouldn't ever be missing an eltType here"); offsets.push_back(0); offsets.push_back(adaptor.getOffset()); @@ -5461,7 +5473,8 @@ mlir::LogicalResult CIRToLLVMMemChrOpLowering::matchAndRewrite( mlir::ArrayAttr argAttrs = mlir::ArrayAttr::get(rewriter.getContext(), argAttrVec); - createLLVMFuncOpIfNotExist(rewriter, op, fnName, fnTy, argAttrs); + createLLVMFuncOpIfNotExist(rewriter, symbolTables, op, fnName, fnTy, + argAttrs); mlir::LLVM::CallOp newCall = rewriter.replaceOpWithNewOp<mlir::LLVM::CallOp>( op, mlir::TypeRange{llvmPtrTy}, fnName, diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h index 5181e52976bfe..146b31b907fcc 100644 --- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h +++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h @@ -29,6 +29,7 @@ struct LLVMBlockAddressInfo; /// block address attributes, `blockInfoAddr` is used to resolve them. mlir::Value lowerCirAttrAsValue(mlir::Operation *parentOp, mlir::Attribute attr, mlir::ConversionPatternRewriter &rewriter, + mlir::SymbolTableCollection &symbolTables, const mlir::TypeConverter *converter, LLVMBlockAddressInfo *blockInfoAddr = nullptr); diff --git a/clang/lib/CIR/Lowering/LoweringHelpers.cpp b/clang/lib/CIR/Lowering/LoweringHelpers.cpp index 1b279a505c95c..59677a1aacefd 100644 --- a/clang/lib/CIR/Lowering/LoweringHelpers.cpp +++ b/clang/lib/CIR/Lowering/LoweringHelpers.cpp @@ -139,9 +139,10 @@ mlir::DenseElementsAttr convertToDenseElementsAttr( /// Return true when \p gv can be lowered to a \c FlatSymbolRefAttr leaf without /// addrspacecast or bitcast (mirrors \c CIRAttrToValue::visitCirAttr). -static bool globalViewMatchesPointerLeaf(cir::GlobalViewAttr gv, - mlir::ModuleOp moduleOp, - const mlir::TypeConverter *converter) { +static bool +globalViewMatchesPointerLeaf(cir::GlobalViewAttr gv, mlir::ModuleOp moduleOp, + mlir::SymbolTableCollection &symbolTables, + const mlir::TypeConverter *converter) { if (gv.getIndices() || mlir::isa<cir::IntType, cir::VPtrType>(gv.getType())) return false; @@ -151,8 +152,7 @@ static bool globalViewMatchesPointerLeaf(cir::GlobalViewAttr gv, unsigned sourceAddrSpace = 0; mlir::Type sourceType; - auto sourceSymbol = - mlir::SymbolTable::lookupSymbolIn(moduleOp, gv.getSymbol()); + auto sourceSymbol = symbolTables.lookupSymbolIn(moduleOp, gv.getSymbol()); if (auto llvmSymbol = mlir::dyn_cast<mlir::LLVM::GlobalOp>(sourceSymbol)) { sourceType = llvmSymbol.getType(); sourceAddrSpace = llvmSymbol.getAddrSpace(); @@ -186,9 +186,11 @@ static bool globalViewMatchesPointerLeaf(cir::GlobalViewAttr gv, static std::optional<mlir::Attribute> lowerPointerElementAttr(mlir::Attribute elt, mlir::MLIRContext *ctx, mlir::ModuleOp moduleOp, + mlir::SymbolTableCollection &symbolTables, const mlir::TypeConverter *converter) { if (auto gv = mlir::dyn_cast<cir::GlobalViewAttr>(elt)) { - if (!moduleOp || !globalViewMatchesPointerLeaf(gv, moduleOp, converter)) + if (!moduleOp || + !globalViewMatchesPointerLeaf(gv, moduleOp, symbolTables, converter)) return std::nullopt; return gv.getSymbol(); } @@ -214,15 +216,13 @@ static bool containsPoison(mlir::Attribute attr) { return false; } -static std::optional<mlir::Attribute> -lowerConstRecordMemberAttr(mlir::Attribute attr, - const mlir::TypeConverter *converter, - mlir::ModuleOp moduleOp); - -std::optional<mlir::Attribute> -lowerConstArrayAttr(cir::ConstArrayAttr constArr, - const mlir::TypeConverter *converter, - mlir::ModuleOp moduleOp) { +static std::optional<mlir::Attribute> lowerConstRecordMemberAttr( + mlir::Attribute attr, mlir::SymbolTableCollection &symbolTables, + const mlir::TypeConverter *converter, mlir::ModuleOp moduleOp); + +std::optional<mlir::Attribute> lowerConstArrayAttr( + cir::ConstArrayAttr constArr, mlir::SymbolTableCollection &symbolTables, + const mlir::TypeConverter *converter, mlir::ModuleOp moduleOp) { // Ensure ConstArrayAttr has a type. const auto typedConstArr = mlir::cast<mlir::TypedAttr>(constArr); @@ -274,7 +274,7 @@ lowerConstArrayAttr(cir::ConstArrayAttr constArr, mlir::MLIRContext *ctx = constArr.getContext(); for (mlir::Attribute elt : eltsArr) { std::optional<mlir::Attribute> llvmElt = - l... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/213972 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
