llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangir Author: Erich Keane (erichkeane) <details> <summary>Changes</summary> The hwasan bot found quite some issues with how we were managing contexts/memory throughout. First, CIRAction doesn't need an MLIRContext at all, and was never being passed one, so it would always construct one, then never erase it. Second in CIRGenModule, the module is non-owning, so we need to make sure we erase it at the end of the module. Third, when generating the global view for a vptr, we were creating an operation with the intent of 'stealing' from it, but never ended up adding it anywhere, so it would be lost. Forth, we had a few uses of getAddrOfGlobalVar that were causing an extra getglobalop when they really just needed the global itself. That caused those to never be emitted right either. Finally, an issue in PointerLikeTest caused us to leak a ConstantOp, wh ich this patch properly wraps in the OwningOpRef. --- Full diff: https://github.com/llvm/llvm-project/pull/234294.diff 6 Files Affected: - (modified) clang/include/clang/CIR/FrontendAction/CIRGenAction.h (+7-9) - (modified) clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp (+5) - (modified) clang/lib/CIR/CodeGen/CIRGenModule.cpp (+3-3) - (modified) clang/lib/CIR/FrontendAction/CIRGenAction.cpp (+9-15) - (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+1-1) - (modified) clang/unittests/CIR/PointerLikeTest.cpp (+2-1) ``````````diff diff --git a/clang/include/clang/CIR/FrontendAction/CIRGenAction.h b/clang/include/clang/CIR/FrontendAction/CIRGenAction.h index 9f8bad248ea47e..93d97450ba898d 100644 --- a/clang/include/clang/CIR/FrontendAction/CIRGenAction.h +++ b/clang/include/clang/CIR/FrontendAction/CIRGenAction.h @@ -44,13 +44,11 @@ class CIRGenAction : public clang::ASTFrontendAction { mlir::OwningOpRef<mlir::ModuleOp> MLIRMod; - mlir::MLIRContext *MLIRCtx; - std::unique_ptr<llvm::LLVMContext> Ctx; llvm::SmallVector<clang::LinkModule> LinkModules; protected: - CIRGenAction(OutputType Action, mlir::MLIRContext *MLIRCtx = nullptr); + CIRGenAction(OutputType Action); bool BeginSourceFileAction(clang::CompilerInstance &CI) override; @@ -72,42 +70,42 @@ class EmitCIRAction : public CIRGenAction { virtual void anchor(); public: - EmitCIRAction(mlir::MLIRContext *MLIRCtx = nullptr); + EmitCIRAction(); }; class EmitCIRBCAction : public CIRGenAction { virtual void anchor(); public: - EmitCIRBCAction(mlir::MLIRContext *MLIRCtx = nullptr); + EmitCIRBCAction(); }; class EmitLLVMAction : public CIRGenAction { virtual void anchor(); public: - EmitLLVMAction(mlir::MLIRContext *MLIRCtx = nullptr); + EmitLLVMAction(); }; class EmitBCAction : public CIRGenAction { virtual void anchor(); public: - EmitBCAction(mlir::MLIRContext *MLIRCtx = nullptr); + EmitBCAction(); }; class EmitAssemblyAction : public CIRGenAction { virtual void anchor(); public: - EmitAssemblyAction(mlir::MLIRContext *MLIRCtx = nullptr); + EmitAssemblyAction(); }; class EmitObjAction : public CIRGenAction { virtual void anchor(); public: - EmitObjAction(mlir::MLIRContext *MLIRCtx = nullptr); + EmitObjAction(); }; } // namespace cir diff --git a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp index 0a93ad586e2238..21bd842c24cd40 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp @@ -378,6 +378,11 @@ mlir::Attribute buildRecordHelper(ConstantEmitter &emitter, elements[0] = cir::GlobalViewAttr::get( cir::VPtrType::get(builder.getContext()), apOp.getNameAttr(), indices, /*addressPoint=*/true); + + // we only used the getVTableAddressPoint call to get the index/offset for + // the indices, and never added it to the module. So we need to erase it + // here. + apOp->erase(); } unsigned baseNo = 0; diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp index 6142ee62f3be11..10ead4174244e2 100644 --- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp @@ -246,7 +246,7 @@ CIRGenModule::CIRGenModule(mlir::MLIRContext &mlirContext, } } -CIRGenModule::~CIRGenModule() = default; +CIRGenModule::~CIRGenModule() { theModule->erase(); } void CIRGenModule::setOpenCLVersionAttr(StringRef attrName, unsigned version) { theModule->setAttr( @@ -629,7 +629,7 @@ void CIRGenModule::emitGlobal(clang::GlobalDecl gd) { // change linkage, make sure that it's emitted. if (astContext.getInlineVariableDefinitionKind(vd) == ASTContext::InlineVariableDefinitionKind::Strong) - getAddrOfGlobalVar(vd); + getOrCreateCIRGlobal(vd, /*ty=*/{}, NotForDefinition); // Otherwise, we can ignore this declaration. The variable will be emitted // on its first use. return; @@ -757,7 +757,7 @@ void CIRGenModule::handleCXXStaticMemberVarInstantiation(VarDecl *vd) { // If we have a definition, this might be a deferred decl. If the // instantiation is explicit, make sure we emit it at the end. if (vd->getDefinition() && tsk == TSK_ExplicitInstantiationDefinition) - getAddrOfGlobalVar(vd); + getOrCreateCIRGlobal(vd, /*ty=*/{}, NotForDefinition); emitTopLevelDecl(vd); } diff --git a/clang/lib/CIR/FrontendAction/CIRGenAction.cpp b/clang/lib/CIR/FrontendAction/CIRGenAction.cpp index 3cdf8f22cea017..c7a826c4805e19 100644 --- a/clang/lib/CIR/FrontendAction/CIRGenAction.cpp +++ b/clang/lib/CIR/FrontendAction/CIRGenAction.cpp @@ -370,9 +370,8 @@ class CIRGenConsumer : public clang::ASTConsumer { void CIRGenConsumer::anchor() {} -CIRGenAction::CIRGenAction(OutputType Act, mlir::MLIRContext *MLIRCtx) - : MLIRCtx(MLIRCtx ? MLIRCtx : new mlir::MLIRContext), - Ctx(std::make_unique<llvm::LLVMContext>()), Action(Act) {} +CIRGenAction::CIRGenAction(OutputType Act) + : Ctx(std::make_unique<llvm::LLVMContext>()), Action(Act) {} CIRGenAction::~CIRGenAction() { MLIRMod.release(); } @@ -492,25 +491,20 @@ CIRGenAction::CreateASTConsumer(CompilerInstance &CI, StringRef InFile) { } void EmitAssemblyAction::anchor() {} -EmitAssemblyAction::EmitAssemblyAction(mlir::MLIRContext *MLIRCtx) - : CIRGenAction(OutputType::EmitAssembly, MLIRCtx) {} +EmitAssemblyAction::EmitAssemblyAction() + : CIRGenAction(OutputType::EmitAssembly) {} void EmitCIRAction::anchor() {} -EmitCIRAction::EmitCIRAction(mlir::MLIRContext *MLIRCtx) - : CIRGenAction(OutputType::EmitCIR, MLIRCtx) {} +EmitCIRAction::EmitCIRAction() : CIRGenAction(OutputType::EmitCIR) {} void EmitCIRBCAction::anchor() {} -EmitCIRBCAction::EmitCIRBCAction(mlir::MLIRContext *MLIRCtx) - : CIRGenAction(OutputType::EmitCIRBC, MLIRCtx) {} +EmitCIRBCAction::EmitCIRBCAction() : CIRGenAction(OutputType::EmitCIRBC) {} void EmitLLVMAction::anchor() {} -EmitLLVMAction::EmitLLVMAction(mlir::MLIRContext *MLIRCtx) - : CIRGenAction(OutputType::EmitLLVM, MLIRCtx) {} +EmitLLVMAction::EmitLLVMAction() : CIRGenAction(OutputType::EmitLLVM) {} void EmitBCAction::anchor() {} -EmitBCAction::EmitBCAction(mlir::MLIRContext *MLIRCtx) - : CIRGenAction(OutputType::EmitBC, MLIRCtx) {} +EmitBCAction::EmitBCAction() : CIRGenAction(OutputType::EmitBC) {} void EmitObjAction::anchor() {} -EmitObjAction::EmitObjAction(mlir::MLIRContext *MLIRCtx) - : CIRGenAction(OutputType::EmitObj, MLIRCtx) {} +EmitObjAction::EmitObjAction() : CIRGenAction(OutputType::EmitObj) {} diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp index ac4e5a9f01c4a5..93c9b30a54d4c8 100644 --- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp +++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp @@ -3349,7 +3349,7 @@ void CIRToLLVMGlobalOpLowering::setupRegionInitializedLLVMGlobalOp( rewriter.replaceOpWithNewOp<mlir::LLVM::GlobalOp>( op, llvmType, isConst, linkage, symbol, nullptr, alignment, addrSpace, isDsoLocal, threadLocalMode, comdatAttr, attributes); - newGlobalOp.getRegion().emplaceBlock(); + rewriter.createBlock(&newGlobalOp.getRegion()); rewriter.setInsertionPointToEnd(newGlobalOp.getInitializerBlock()); } diff --git a/clang/unittests/CIR/PointerLikeTest.cpp b/clang/unittests/CIR/PointerLikeTest.cpp index 8e5dc6636e01e2..13569b3931d69f 100644 --- a/clang/unittests/CIR/PointerLikeTest.cpp +++ b/clang/unittests/CIR/PointerLikeTest.cpp @@ -140,8 +140,9 @@ class CIROpenACCPointerLikeTest : public ::testing::Test { // Create an element access. mlir::Type i32Ty = cir::IntType::get(&context, 32, true); - mlir::Value index = + OwningOpRef<cir::ConstantOp> indexOp = cir::ConstantOp::create(b, loc, cir::IntAttr::get(i32Ty, 2)); + mlir::Value index = indexOp.get(); OwningOpRef<cir::PtrStrideOp> accessPtr = cir::PtrStrideOp::create(b, loc, ptrToElemTy, decayVal, index); mlir::Value accessVal = accessPtr.get(); `````````` </details> https://github.com/llvm/llvm-project/pull/234294 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
