llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Andy Kaylor (andykaylor) <details> <summary>Changes</summary> When a class contains virtual functions but no data members and has a trivial constructor, global variables of that type are initialized with a vptr. CIR was incorrectly creating the global variable with the type of the vtable (an anonymous record) rather than the class type. When replacing structors with aliases, we were calling a function to update argument types at the call sites, but this was only necessary because we initially generated the call using the same incorrect type that we used for the global. The type correction wasn't implemented because we hadn't encountered a case where it was needed. Having found such a case led me to diagnose the problem as above, and I verified that the same test case compiled without -mconstructor-aliases just failed in the verifier because we never hit the replacement code. I'm now convinced that this argument type fixup isn't necessary, so I replaced the fixup function with an assert. Assisted-by: Cursor / claude-4.6-opus-high --- Full diff: https://github.com/llvm/llvm-project/pull/191291.diff 6 Files Affected: - (modified) clang/include/clang/CIR/Dialect/IR/CIRTypes.td (+1) - (modified) clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp (+3-1) - (modified) clang/lib/CIR/CodeGen/CIRGenModule.cpp (+13-15) - (modified) clang/lib/CIR/CodeGen/CIRGenModule.h (-5) - (modified) clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp (+3) - (added) clang/test/CIR/CodeGen/global-ptr-init.cpp (+16) ``````````diff diff --git a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td index 778f9bfdfd0c1..b177eab08ccad 100644 --- a/clang/include/clang/CIR/Dialect/IR/CIRTypes.td +++ b/clang/include/clang/CIR/Dialect/IR/CIRTypes.td @@ -362,6 +362,7 @@ def CIR_DataMemberType : CIR_Type<"DataMember", "data_member", [ //===----------------------------------------------------------------------===// def CIR_VPtrType : CIR_Type<"VPtr", "vptr", [ + DeclareTypeInterfaceMethods<CIR_SizedTypeInterface>, DeclareTypeInterfaceMethods<DataLayoutTypeInterface> ]> { let summary = "CIR type that is used for the vptr member of C++ objects"; diff --git a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp index fe9a8ee1957c4..330e51c214dfc 100644 --- a/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp @@ -751,8 +751,10 @@ bool ConstRecordBuilder::build(const APValue &val, const RecordDecl *rd, builder.getI32IntegerAttr(addressPoint.VTableIndex), builder.getI32IntegerAttr(addressPoint.AddressPointIndex), }); + auto vptrTy = cir::VPtrType::get(cgm.getBuilder().getContext()); + auto symbol = mlir::FlatSymbolRefAttr::get(vtable.getSymNameAttr()); cir::GlobalViewAttr vtableInit = - cgm.getBuilder().getGlobalViewAttr(vtable, indices); + cir::GlobalViewAttr::get(vptrTy, symbol, indices); if (!appendBytes(offset, vtableInit)) return false; } diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp index 2037b92e2b5d1..3f0f493f5a50b 100644 --- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp +++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp @@ -1441,30 +1441,29 @@ void CIRGenModule::addReplacement(StringRef name, mlir::Operation *op) { replacements[name] = op; } -void CIRGenModule::replacePointerTypeArgs(cir::FuncOp oldF, cir::FuncOp newF) { +#ifndef NDEBUG +static bool verifyPointerTypeArgs(mlir::ModuleOp modOp, cir::FuncOp oldF, + cir::FuncOp newF) { std::optional<mlir::SymbolTable::UseRange> optionalUseRange = - oldF.getSymbolUses(theModule); + oldF.getSymbolUses(modOp); if (!optionalUseRange) - return; + return true; for (const mlir::SymbolTable::SymbolUse &u : *optionalUseRange) { - // CallTryOp only shows up after FlattenCFG. auto call = mlir::dyn_cast<cir::CallOp>(u.getUser()); if (!call) continue; - for (const auto [argOp, fnArgType] : + for (auto [argOp, fnArgType] : llvm::zip(call.getArgs(), newF.getFunctionType().getInputs())) { - if (argOp.getType() == fnArgType) - continue; - - // The purpose of this entire function is to insert bitcasts in the case - // where these types don't match, but I haven't seen a case where that - // happens. - errorNYI(call.getLoc(), "replace call with mismatched types"); + if (argOp.getType() != fnArgType) + return false; } } + + return true; } +#endif // NDEBUG void CIRGenModule::applyReplacements() { for (auto &i : replacements) { @@ -1482,9 +1481,8 @@ void CIRGenModule::applyReplacements() { continue; } - // LLVM has opaque pointer but CIR not. So we may have to handle these - // different pointer types when performing replacement. - replacePointerTypeArgs(oldF, newF); + assert(verifyPointerTypeArgs(theModule, oldF, newF) && + "call argument types do not match replacement function"); // Replace old with new, but keep the old order. if (oldF.replaceAllSymbolUses(newF.getSymNameAttr(), theModule).failed()) diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.h b/clang/lib/CIR/CodeGen/CIRGenModule.h index 266510de84fd0..0bb07f0815d43 100644 --- a/clang/lib/CIR/CodeGen/CIRGenModule.h +++ b/clang/lib/CIR/CodeGen/CIRGenModule.h @@ -799,11 +799,6 @@ class CIRGenModule : public CIRGenTypeCache { /// Call replaceAllUsesWith on all pairs in replacements. void applyReplacements(); - /// A helper function to replace all uses of OldF to NewF that replace - /// the type of pointer arguments. This is not needed to tradtional - /// pipeline since LLVM has opaque pointers but CIR not. - void replacePointerTypeArgs(cir::FuncOp oldF, cir::FuncOp newF); - void setNonAliasAttributes(GlobalDecl gd, mlir::Operation *op); /// Map source language used to a CIR attribute. diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp index 2117dd5903ec4..3fe63398793f7 100644 --- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp +++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp @@ -600,6 +600,9 @@ mlir::Value CIRAttrToValue::visitCirAttr(cir::GlobalViewAttr globalAttr) { llvmDstTy, addrOp); } + if (mlir::isa<cir::VPtrType>(globalAttr.getType())) + return addrOp; + llvm_unreachable("Expecting pointer or integer type for GlobalViewAttr"); } diff --git a/clang/test/CIR/CodeGen/global-ptr-init.cpp b/clang/test/CIR/CodeGen/global-ptr-init.cpp new file mode 100644 index 0000000000000..1520485cf3597 --- /dev/null +++ b/clang/test/CIR/CodeGen/global-ptr-init.cpp @@ -0,0 +1,16 @@ +// RUN: %clang_cc1 -triple=x86_64-linux-gnu -std=gnu++14 -fclangir -emit-cir -o %t.cir %s +// RUN: FileCheck -check-prefix=CIR --input-file=%t.cir %s +// RUN: %clang_cc1 -triple=x86_64-linux-gnu -std=gnu++14 -fclangir -emit-llvm -o %t-cir.ll %s +// RUN: FileCheck -check-prefix=LLVM --input-file=%t-cir.ll %s +// RUN: %clang_cc1 -triple=x86_64-linux-gnu -std=gnu++14 -emit-llvm -o %t.ll %s +// RUN: FileCheck -check-prefix=OGCG --input-file=%t.ll %s + +struct B { + virtual void f() {} + virtual ~B() {} +}; +B x; + +// CIR: cir.global external @x = #cir.const_record<{#cir.global_view<@_ZTV1B, [0 : i32, 2 : i32]> : !cir.vptr}> : !rec_B +// LLVM: @x = global %struct.B { ptr getelementptr inbounds nuw (i8, ptr @_ZTV1B, i64 16) }, align 8 +// OGCG: @x = global %struct.B { ptr getelementptr inbounds inrange(-16, 24) ({ [5 x ptr] }, ptr @_ZTV1B, i32 0, i32 0, i32 2) }, align 8 `````````` </details> https://github.com/llvm/llvm-project/pull/191291 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
