I’m running into some rough edges when trying to deal with LLVM dialect symbols. The issue is that though ops like LLVMFuncOp and LLVMGlobalOp implement the MLIR Symbol interface they don’t provide a meaningful implementation of MLIR’s symbol visibility. This makes it difficult to write passes that operate on modules with LLVM dialect ops in them without special-casing to the LLVM dialect and checking the linkage on the ops.
I’m not sure there’s any particular solution to this but wanted to raise the issue in case others had any ideas. One is that symbol visibility could be moved to its own SymbolVisibility interface so that at least there’s a way to know when a Symbol is going to ignore visibility. Another is that the LLVM ops could at least satisfy the isPublic/isPrivate/etc interface queries based on the LLVM linkage. There are also some defaults to linkage in the LLVM dialect and those may be enough to allow setVisibility to map from the more limited set of MLIR visibilities to LLVM linkages.
Any thoughts @ftynse? The linkage stuff went in 2 years ago - possibly before there even was symbol visibility in MLIR - and I’m wondering if there’s anything you’d change if doing it today.
LLVM linkage indeed predates symbol visibility and maybe even symbols. Symbol visibility doesn’t seem to have been designed with LLVM dialect in mind and the dialect was never updated.
I don’t fully grasp what is the root of the problem here. If you have a flow that creates LLVM dialect, it should be possible to set up both linkage and visibility attributes when creating the ops using the mapping between them that makes most sense. This can be also done in some “inference” pass. If you want the MLIR symbol visibility to be somehow reflected in the LLVM IR, some lowering from a higher level (if any) to the LLVM dialect should account for visibility when setting up the linkage.
On the first thought, I would suggest inferring the smaller set of options (visibility) from the larger one (linkage) though. Both private and internal linkage sound like they should map to private visibility, but the inverse conversion would be ambiguous and require us to define some default convention.
Good point. I think the thing I was hoping was that I could take a generic MLIR pass that operated on symbols and run it on a module after lowering to the LLVM dialect - today that’s not possible if the pass uses any information derived from symbol visibility. When composing a complex set of pipelines - some of which may mutate the IR - it’d be nice not to have such major points where “no more MLIR passes can be run after this point, even though you still have MLIR IR that uses MLIR types and may have other dialects mixed into it that you do want to run those passes on.”
It’s possible to make a policy decision and say “you can’t run any pass that uses symbol visibility information on an LLVM dialect module” but that’s a big footgun/surprise that would be nice to disallow via construction (like splitting off visibility from the symbol interface). Having a dedicated llvm.module would help as there’d be less expectation that I could run a pass specified for ModuleOps on an LLVMModuleOp. It’s like how not being able to run MLIR passes once exported to an llvm::Module isn’t confusing because they are different type systems. Today the only way to know you can’t run MLIR passes on a module is if it contains llvm.func ops, which is pretty nasty and hard to specify if mixing multiple dialects.
So I guess some of this may dovetail with De-privileging ModuleOp in translation APIs and making llvm.module a real thing - then only being able to run passes that are LLVM-dialect aware would negate the need for shared visibility though it would lead to some duplication (for example would need a new SymbolDCE pass just for the LLVM dialect, etc).
That seems to break major invariant in MLIR, I would expect that passes that rely on MLIR symbol visibility (but do not modify them) can operate on something like the LLVM dialect (SymbolDCE that you mention for instance).
Is this just a matter to have an interface all of what’s missing here? I guess the alternative is to duplicate informations from the llvm dialect linkage into the MLIR visibility attribute and keep this consistent…
That’d be best IMO - but would be nice to enforce the immutability somehow if it is not always possible to do (like mapping MLIR visibility to LLVM linkage may not be).
I’m not sure what you mean here. Linkage and visibility are related, but different concepts. You can’t (reasonably) map visibility to linkage, because linkage carries a notion of how to handle the symbol during linking (which is understandably not a universal concept in MLIR). MLIR visibility is intended to describe how an MLIR symbol can be referenced within the IR (note that this is not exactly the same as LLVM visibility). When lowering to LLVM IR one could conceptually infer MLIR visibility from the LLVM linkage+visibility, but there won’t be a direct mapping given that LLVM has no notion of “nested” (which makes sense given its design scope). With that being said, the only other main thing the LLVM dialect will have to do is have a definition of canDiscardOnUseEmpty that takes into account LLVM linkage.
Yes - that’s why I say it may not be possible to alias them
I’m pointing out that SymbolOpInterface has setVisibility and if on some symbols that does nothing (isPublic() == true, setVisibility(Private), isPublic() == true) that seems like a footgun that should not be allowed (verification in LLVM dialect that funcs don’t have sym_visibility attrs may be enough).
I still don’t understand what is the issue here. LLVM functions are symbols and passes that work on symbol interfaces seem to work on them. I just tried the following:
OpBuilder builder(module->getBody(), module->getBody()->end());
auto type = LLVM::LLVMFunctionType::get(LLVM::LLVMVoidType::get(context), {});
auto func =
builder.create<LLVM::LLVMFuncOp>(builder.getUnknownLoc(), "foo", type);
func.dump();
llvm::errs() << cast<SymbolOpInterface>(func.getOperation()).isPublic() << "\n";
cast<SymbolOpInterface>(func.getOperation()).setPrivate();
func.dump();
llvm::errs() << cast<SymbolOpInterface>(func.getOperation()).isPublic() << "\n";
