Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CIR][Interface] introduce CIRGlobalValueInterface for GlobalOp and FuncOp #641

Merged
merged 1 commit into from
May 31, 2024
Merged

[CIR][Interface] introduce CIRGlobalValueInterface for GlobalOp and FuncOp #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 8 additions & 15 deletions clang/include/clang/CIR/Dialect/IR/CIROps.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1872,7 +1872,10 @@ def TLSModel : I32EnumAttr<
let cppNamespace = "::mlir::cir";
}

def GlobalOp : CIR_Op<"global", [Symbol, DeclareOpInterfaceMethods<RegionBranchOpInterface>, NoRegionArguments]> {
def GlobalOp : CIR_Op<"global",
[DeclareOpInterfaceMethods<RegionBranchOpInterface>,
DeclareOpInterfaceMethods<CIRGlobalValueInterface>,
NoRegionArguments]> {
let summary = "Declares or defines a global variable";
let description = [{
The `cir.global` operation declares or defines a named global variable.
Expand Down Expand Up @@ -1933,13 +1936,6 @@ def GlobalOp : CIR_Op<"global", [Symbol, DeclareOpInterfaceMethods<RegionBranchO
bool hasAvailableExternallyLinkage() {
return mlir::cir::isAvailableExternallyLinkage(getLinkage());
}
bool isDeclarationForLinker() {
if (hasAvailableExternallyLinkage())
return true;

return isDeclaration();
}

/// Whether the definition of this global may be replaced at link time.
bool isWeakForLinker() { return cir::isWeakForLinker(getLinkage()); }
}];
Expand Down Expand Up @@ -2585,7 +2581,8 @@ def BaseClassAddrOp : CIR_Op<"base_class_addr"> {

def FuncOp : CIR_Op<"func", [
AutomaticAllocationScope, CallableOpInterface, FunctionOpInterface,
IsolatedFromAbove, Symbol
DeclareOpInterfaceMethods<CIRGlobalValueInterface>,
IsolatedFromAbove
]> {
let summary = "Declare or define a function";
let description = [{
Expand Down Expand Up @@ -2727,12 +2724,8 @@ def FuncOp : CIR_Op<"func", [

bool isDeclaration();

// FIXME: should be shared with GlobalOp extra declaration.
bool isDeclarationForLinker() {
if (mlir::cir::isAvailableExternallyLinkage(getLinkage()))
return true;

return isDeclaration();
bool hasAvailableExternallyLinkage() {
return mlir::cir::isAvailableExternallyLinkage(getLinkage());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given both hasAvailableExternallyLinkage impls are equal, just make this the default impl in the td file, and delete the one from GlobalOp.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually tried that. But it fails to compile as tbgen doesn't recognize getLinkage(), and getLinkage() seems to be a getter for OP attribute "linkage". Would love to learn how we deal with this situation as I totally agree with you this code dup is ugly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something along the lines of what we see in CIR_CallOp could be doable:

    /// Return the callee of this operation
    CallInterfaceCallable getCallableForCallee() {
      return (*this)->getAttrOfType<SymbolRefAttr>("callee");
    }

Note that because follow up work will make mlir::cir::isAvailableExternallyLinkage be part of the interface as well, perhaps you can address this as part of that change instead.

}
}];

Expand Down
35 changes: 30 additions & 5 deletions clang/include/clang/CIR/Interfaces/CIROpInterfaces.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,49 @@
#define MLIR_CIR_OP_INTERFACES

include "mlir/IR/OpBase.td"
include "mlir/IR/SymbolInterfaces.td"
include "mlir/Interfaces/CallInterfaces.td"

let cppNamespace = "::mlir::cir" in {
// The CIRCallOpInterface must be used instead of CallOpInterface when looking
// at arguments and other bits of CallOp. This creates a level of abstraction
// that's useful for handling indirect calls and other details.
def CIRCallOpInterface : OpInterface<"CIRCallOpInterface", [CallOpInterface]> {
def CIRCallOpInterface
: OpInterface<"CIRCallOpInterface", [CallOpInterface]> {
let methods = [
InterfaceMethod<"", "mlir::Operation::operand_iterator",
"arg_operand_begin", (ins)>,
InterfaceMethod<"", "mlir::Operation::operand_iterator",
"arg_operand_end", (ins)>,
InterfaceMethod<
"Return the operand at index 'i', accounts for indirect call or "
"exception info", "mlir::Value", "getArgOperand", (ins "unsigned":$i)>,
"Return the operand at index 'i', accounts for indirect call or "
"exception info",
"mlir::Value", "getArgOperand",
(ins "unsigned"
: $i)>,
InterfaceMethod<
"Return the number of operands, accounts for indirect call or "
"exception info", "unsigned", "getNumArgOperands", (ins)>,
"Return the number of operands, accounts for indirect call or "
"exception info",
"unsigned", "getNumArgOperands", (ins)>,
];
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice formatting, thanks. Note that changes to CIRCallOpInterface are unrelated to the patch and makes it harder to review. Fine for this PR because you didn't know but please avoid doing that in the future, better do formatting/refactoring on their own PRs.


def CIRGlobalValueInterface
: OpInterface<"CIRGlobalValueInterface", [Symbol]> {

let methods = [
InterfaceMethod<"",
"bool", "hasAvailableExternallyLinkage", (ins), [{}],
/*defaultImplementation=*/[{ return false; }]
>,
InterfaceMethod<"",
"bool", "isDeclarationForLinker", (ins), [{}],
/*defaultImplementation=*/[{
if ($_op.hasAvailableExternallyLinkage())
return true;
return $_op.isDeclaration();
}]
>,
];
}

Expand Down
11 changes: 6 additions & 5 deletions clang/lib/CIR/CodeGen/CIRGenCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,18 +122,19 @@ bool CIRGenModule::tryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
// Check if we have it already.
StringRef MangledName = getMangledName(AliasDecl);
auto Entry = getGlobalValue(MangledName);
auto fnOp = dyn_cast_or_null<mlir::cir::FuncOp>(Entry);
if (Entry && fnOp && !fnOp.isDeclaration())
auto globalValue = dyn_cast<mlir::cir::CIRGlobalValueInterface>(Entry);
if (Entry && globalValue && !globalValue.isDeclaration())
return false;
if (Replacements.count(MangledName))
return false;

assert(fnOp && "only knows how to handle FuncOp");
assert(globalValue && "only knows how to handle GlobalValue");
[[maybe_unused]] auto AliasValueType = getTypes().GetFunctionType(AliasDecl);

// Find the referent.
auto Aliasee = cast<mlir::cir::FuncOp>(GetAddrOfGlobal(TargetDecl));

auto AliaseeGV = dyn_cast_or_null<mlir::cir::CIRGlobalValueInterface>(
GetAddrOfGlobal(TargetDecl));
// Instead of creating as alias to a linkonce_odr, replace all of the uses
// of the aliasee.
if (mlir::cir::isDiscardableIfUnused(Linkage) &&
Expand Down Expand Up @@ -161,7 +162,7 @@ bool CIRGenModule::tryEmitBaseDestructorAsAlias(const CXXDestructorDecl *D) {
// is
// avaialable_externally, don't emit an alias. We can't emit aliases to
// declarations; that's just not how aliases work.
if (Aliasee.isDeclarationForLinker())
if (AliaseeGV && AliaseeGV.isDeclarationForLinker())
return true;

// Don't create an alias to a linker weak symbol. This avoids producing
Expand Down
17 changes: 12 additions & 5 deletions clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,10 +448,14 @@ static void emitConstructorDestructorAlias(CIRGenModule &CGM,

// Does this function alias already exists?
StringRef MangledName = CGM.getMangledName(AliasDecl);
auto globalValue = dyn_cast_or_null<mlir::cir::CIRGlobalValueInterface>(
CGM.getGlobalValue(MangledName));
if (globalValue && !globalValue.isDeclaration()) {
return;
}

auto Entry =
dyn_cast_or_null<mlir::cir::FuncOp>(CGM.getGlobalValue(MangledName));
if (Entry && !Entry.isDeclaration())
return;

// Retrieve aliasee info.
auto Aliasee =
Expand Down Expand Up @@ -2051,19 +2055,22 @@ void CIRGenItaniumCXXABI::emitVTableDefinitions(CIRGenVTables &CGVT,
// EmitFundamentalRTTIDescriptors(RD);
}

auto VTableAsGlobalValue =
dyn_cast<mlir::cir::CIRGlobalValueInterface>(*VTable);
assert(VTableAsGlobalValue && "VTable must support CIRGlobalValueInterface");
bool isDeclarationForLinker = VTableAsGlobalValue.isDeclarationForLinker();
// Always emit type metadata on non-available_externally definitions, and on
// available_externally definitions if we are performing whole program
// devirtualization. For WPD we need the type metadata on all vtable
// definitions to ensure we associate derived classes with base classes
// defined in headers but with a strong definition only in a shared
// library.
if (!VTable.isDeclarationForLinker() ||
CGM.getCodeGenOpts().WholeProgramVTables) {
if (!isDeclarationForLinker || CGM.getCodeGenOpts().WholeProgramVTables) {
CGM.buildVTableTypeMetadata(RD, VTable, VTLayout);
// For available_externally definitions, add the vtable to
// @llvm.compiler.used so that it isn't deleted before whole program
// analysis.
if (VTable.isDeclarationForLinker()) {
if (isDeclarationForLinker) {
llvm_unreachable("NYI");
assert(CGM.getCodeGenOpts().WholeProgramVTables);
assert(!UnimplementedFeature::addCompilerUsedGlobal());
Expand Down
26 changes: 13 additions & 13 deletions clang/lib/CIR/CodeGen/CIRGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,11 +469,12 @@ void CIRGenModule::buildGlobalFunctionDefinition(GlobalDecl GD,
Op = GetAddrOfFunction(GD, Ty, /*ForVTable=*/false, /*DontDefer=*/true,
ForDefinition);

auto Fn = cast<mlir::cir::FuncOp>(Op);
// Already emitted.
if (!Fn.isDeclaration())
auto globalVal = dyn_cast_or_null<mlir::cir::CIRGlobalValueInterface>(Op);
if (globalVal && !globalVal.isDeclaration()) {
// Already emitted.
return;

}
auto Fn = cast<mlir::cir::FuncOp>(Op);
setFunctionLinkage(GD, Fn);
setGVProperties(Op, D);
// TODO(cir): MaubeHandleStaticInExternC
Expand Down Expand Up @@ -2434,18 +2435,17 @@ void CIRGenModule::buildGlobalDecl(clang::GlobalDecl &D) {
// ways (e.g. by an extern inline function acquiring a strong function
// redefinition). Just ignore those cases.
// TODO: Not sure what to map this to for MLIR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete the TODO as well. I think this whole change could be further simplified by:

auto cirGlobalValue = cast<mlir::cir::CIRGlobalValueInterface>(globalValueOp);
if (!cirGlobalValue.isDeclaration())
  return;

See the original code in CodeGenModule::EmitDeferred().

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to see if I understand it correctly. Does this mean "dyn_castmlir::cir::GetGlobalOp(Op)" should not be a valid check here? I.E. early logic in the function should make sure Op is of CIRGlobalValueInterface (either GlobalOp or FuncOp)
Actually, I had that doubt when I changed this code,
now as you suggested, after comparing with code in lib/clang/CodeGen/CodeGenModule.cpp
such as
void CodeGenModule::EmitDeferred() {
and CodeGenModule::GetAddrOfGlobalVar

I kinda question why CIR counterpart CIRGenModule::getAddrOfGlobalVar is returning GetGlobalOp insteadof GlobalOp?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I read GetGlobalOp as GlobalOp, I take back what I said :)

I kinda question why CIR counterpart CIRGenModule::getAddrOfGlobalVar is returning GetGlobalOp insteadof GlobalOp?

The OG one returns llvm::Constant *, which if we think in CIR terms, there isn't a good direct counterpart - it could be either and attribute (globalview) or an actual mlir::value result of getglobalop, so not really translatable at the time we wrote this.

If you have better ideas on how to go about this, it's definitely an improvement we could do in its own PR.

if (auto Fn = dyn_cast<mlir::cir::FuncOp>(Op))
if (!Fn.isDeclaration())
return;

// TODO(cir): create a global value trait that allow us to uniformly handle
// global variables and functions.
auto globalValueOp = Op;
if (auto Gv = dyn_cast<mlir::cir::GetGlobalOp>(Op)) {
auto *result =
mlir::SymbolTable::lookupSymbolIn(getModule(), Gv.getNameAttr());
if (auto globalOp = dyn_cast<mlir::cir::GlobalOp>(result))
if (!globalOp.isDeclaration())
return;
globalValueOp = result;
}

if (auto cirGlobalValue =
dyn_cast<mlir::cir::CIRGlobalValueInterface>(globalValueOp)) {
if (!cirGlobalValue.isDeclaration())
return;
}

// If this is OpenMP, check if it is legal to emit this global normally.
Expand Down
Loading