[mlir] Add pass to add comdat to all linkonce functions #65270
Conversation
|
I'm not really sure this is the best way to do it in general, I wanted to post for comments really. I think a linkage-like comdat attribute on FuncOp would possibly be better in the long run so that languages/conversions that need this behaviour can add it granularly, although we may still want the option to add it for everything for users that don't care and just want linking to work. Doing it like this does fix our use case in flang, as we would be one of the aforementioned non-bothered users :) |
|
The exact use case in flang is that we just want every function that has linkonce or linkonce_odr linkage to have the Any comdat on any target that supports comdats (well, particularly on Windows where linking won't work at all without them, but behaviour is also slightly better on ELF-based platforms with comdats than without them). We particularly see this when converting using the MathToFuncs dialects where ipowi operations are converted using a function defined by the conversion with linkonce linkage, and so these fail to link on Windows with the default linker because it bails on the duplicate definitions with no comdats. If comdats could be added to FuncOps at any point, MathToFuncs could just add the comdat when compiling for a target that supports comdats. |
|
As regards auto-detecting this based on the target somehow, that was suggested in https://reviews.llvm.org/D127455 but was rejected as an approach as not wanting to default to that for everyone and give each frontend more fine-grained control. A blanket option is still quite a blunt instrument but is sufficient for a lot of use cases (including flang's), but I agree with the attached review that it should be an option rather than always forced and auto-detected. |
|
Can you make this a separate pass? |
DavidTruby
changed the title
| @@ -0,0 +1,17 @@ | |||
| // RUN: mlir-opt -llvm-add-comdats -verify-diagnostics %s | FileCheck %s | |||
There was a problem hiding this comment.
Should this test reside in mlir/test/Dialect/LLVMIR/ ?
| @@ -0,0 +1,78 @@ | |||
| //===- AddComdats.cpp - Prepare for translation to LLVM IR ----------------===// | |||
There was a problem hiding this comment.
Nit: The description here sounds odd.
| RewritePatternSet patterns(ctx); | ||
| patterns.add<AddComdat>(ctx); | ||
| if (failed( | ||
| applyPatternsAndFoldGreedily(getOperation(), std::move(patterns)))) |
There was a problem hiding this comment.
Using patterns is far too expensive and overkill for what is a simple traversal here I think.
Seems like this Pass should:
- run on a SymbolOpInterface (most of the time the module).
- Build a symbol table once and for all.
- Iterated on the immediately nested operation and do a single traversal.
There was a problem hiding this comment.
I wasn't sure how to run just on the SymbolOpInterface, if I specify that in Passes.td then things don't compile.
It seems to work just traversing over the module though.
There was a problem hiding this comment.
You are still missing on 2. : "Build a symbol table once and for all"
| void runOnOperation() override { | ||
| OpBuilder builder{&getContext()}; | ||
| ModuleOp mod = getOperation(); | ||
| mod.walk([&](LLVM::LLVMFuncOp op) { |
There was a problem hiding this comment.
The walk is recursive here, we should just iterate: for (auto op = mod.getOperations<LLVM::LLVMFuncOp>())
| RewritePatternSet patterns(ctx); | ||
| patterns.add<AddComdat>(ctx); | ||
| if (failed( | ||
| applyPatternsAndFoldGreedily(getOperation(), std::move(patterns)))) |
There was a problem hiding this comment.
You are still missing on 2. : "Build a symbol table once and for all"
| ModuleOp &module) { | ||
| const char *comdatName = "__llvm_comdat"; | ||
| mlir::LLVM::ComdatOp comdatOp = | ||
| module.lookupSymbol<mlir::LLVM::ComdatOp>(comdatName); |
There was a problem hiding this comment.
This lookup should use the symbolTable built in runOnOperation
| @@ -45,12 +46,13 @@ struct AddComdatsPass : public LLVM::impl::LLVMAddComdatsBase<AddComdatsPass> { | |||
| void runOnOperation() override { | |||
| OpBuilder builder{&getContext()}; | |||
| ModuleOp mod = getOperation(); | |||
| mod.walk([&](LLVM::LLVMFuncOp op) { | |||
| SymbolTable symbolTable{mod}; | |||
There was a problem hiding this comment.
One slight more tweak, can we make this lazy? Maybe:
std::unique_ptr<SymbolTable> symbolTable;
auto getSymTab = [&] () -> SymbolTable& {
if (!symbolTable)
symbolTable = std::make_unique<SymbolTable>(mod);
return *symbolTable;
};
And then you can call addComdat(op, builder, getSymTab(), mod);
This adds a pass operating on the LLVMIR dialect to add an Any comdat to each linkonce and linkonce_odr function when lowering. These comdats are necessary on Windows to allow the default system linker to link binaries containing these functions.
DavidTruby
force-pushed
the
func.comdat
branch
from
361e8f6 to
f7c55e2
Compare
|
The Windows CI seems broken on all patches so I've merged regardless.
No problem, I hadn't written an MLIR pass before so it's good to learn the correct way of doing things! Thanks for the comments. |
This adds a new pass to add an Any comdat to each linkonce and linkonce_odr function in the LLVM dialect. These comdats are necessary on Windows to allow the default system linker to link binaries containing these functions.
This adds a new pass to add an Any comdat to each linkonce and linkonce_odr function in the LLVM dialect. These comdats are necessary on Windows to allow the default system linker to link binaries containing these functions. (cherry picked from commit a685715)
This adds a new pass to add an Any comdat to each linkonce
and linkonce_odr function in the LLVM dialect. These comdats are necessary on Windows
to allow the default system linker to link binaries containing these functions.