Page MenuHomePhabricator

[MLIR] Primitive linkage lowering of FuncOp
ClosedPublic

Authored by wsmoses on Sun, Aug 22, 1:49 PM.

Details

Summary

FuncOp always lowers to an LLVM external linkage presently. This makes it impossible to define functions in mlir which are local to the current module. Until MLIR FuncOps have a more formal linkage specification, this commit allows funcop's that are already marked as private and not public to be internal upon lowering.

Diff Detail

Event Timeline

wsmoses created this revision.Sun, Aug 22, 1:49 PM
wsmoses requested review of this revision.Sun, Aug 22, 1:49 PM

Tests? (I would assume eventually funcOp needs all linkage kinds attached but this is an improvement nevertheless)

wsmoses updated this revision to Diff 368792.Wed, Aug 25, 7:13 PM

Change to llvm linkage rather than just private/public

mehdi_amini added inline comments.Wed, Aug 25, 8:26 PM
mlir/test/Conversion/StandardToLLVM/convert-funcs.mlir
41

Can we use strings instead of integer here? This is just unreadable as is.

ftynse accepted this revision.Thu, Aug 26, 2:04 AM
ftynse added inline comments.
mlir/test/Conversion/StandardToLLVM/convert-funcs.mlir
41

+1. We have helper functions LLVM::symbolize/stringifyLinkage to convert between integers and canonical string representations.

This revision is now accepted and ready to land.Thu, Aug 26, 2:04 AM

Trying to add the stringify/symbolize presents linkage issues, ironically. Both of those functions are (reasonable) defined only within the LLVM dialect. The parser/printer for funcop, however, is within the builtin dialect -- which means that things can't be linked if the helper code is used. Alternatively the llvm linkage attributes could be moved to the builtin dialect, but that's its own can of worms.

Thoughts on how to proceed?

mehdi_amini added a comment.EditedWed, Sep 1, 7:27 PM

I don't quite follow? The printer for func seems irrelevant to me here, this is about the parser/printer for the attribute itself: we need an attribute in the LLVM dialect instead of a builtin attribute and that should just work?

ftynse added a comment.EditedThu, Sep 2, 1:26 AM

The parser/printer for funcop, however, is within the builtin dialect -- which means that things can't be linked if the helper code is used. Alternatively the llvm linkage attributes could be moved to the builtin dialect, but that's its own can of worms.

You shouldn't be needing to modify the assembly of funcop, just attach llvm.linkage as an (unverified by op) string attribute. Then symbolize it in StandardToLLVM.cpp, which does depend on the LLVM dialect, just before passing it to LLVMFunc constructor. You can also add a verifier to the LLVM dialect that checks attributes with llvm.linkage name to only have supported strings.

I don't quite follow? The printer for func seems irrelevant to me here, this is about the parser/printer for the attribute itself: we need an attribute in the LLVM dialect instead of a builtin attribute and that should just work?

The dialect level attribute printer/parser is triggered for attribute types that belong to the dialect (e.g., #llvm.linkage), not attribute names. Enum attributes are just IntegerAttr that are sugared by the printer/parser of the operation that uses them most of the time.

I don't quite follow? The printer for func seems irrelevant to me here, this is about the parser/printer for the attribute itself: we need an attribute in the LLVM dialect instead of a builtin attribute and that should just work?

The dialect level attribute printer/parser is triggered for attribute types that belong to the dialect (e.g., #llvm.linkage), not attribute names. Enum attributes are just IntegerAttr that are sugared by the printer/parser of the operation that uses them most of the time.

I think we're saying the same thing? (I wrote we need an attribute in the LLVM dialect instead of a builtin attribute, the current diff shows an i64)

ftynse added a comment.Thu, Sep 2, 9:54 AM

I don't quite follow? The printer for func seems irrelevant to me here, this is about the parser/printer for the attribute itself: we need an attribute in the LLVM dialect instead of a builtin attribute and that should just work?

The dialect level attribute printer/parser is triggered for attribute types that belong to the dialect (e.g., #llvm.linkage), not attribute names. Enum attributes are just IntegerAttr that are sugared by the printer/parser of the operation that uses them most of the time.

I think we're saying the same thing? (I wrote we need an attribute in the LLVM dialect instead of a builtin attribute, the current diff shows an i64)

I'm fine with this being a string attribute (func @foo() attributes { llvm.linkage = "internal" }) as opposed to introducing a new type specifically for llvm linkage (which is currently an tablegen-backed i64 "enum" attribute pretty-printed as strings by llvm.func assembly, and changing all this is a lot of churn).

OK I misread what you were saying. I would prefer a first class attribute: string looks too much like JSON to me. It would also avoid having to do a conversion in StandardToLLVM which does not seem right for this.

wsmoses updated this revision to Diff 370384.Thu, Sep 2, 1:19 PM

Custom LLVM linkage attribute parsing

How does the following work as a scheme -- adding a custom printer/parser to the LLVM Linkage attribute itself. For a number of specific reasons, I'm not sure of a better way to do so than above. Specifically the tablegen-generated generatedAttributeParser and generatedAttributePrinter don't include any enum attributes since they are themselves subattributes of IntegerAttr and thus aren't registered to the dialect.

You can look at the llvm.fastmath flags for an example (it is defined as LLVM_FMFAttr in LLVMOps.td I believe.

(and void FMFAttr::print(DialectAsmPrinter &printer) and Attribute FMFAttr::parse(MLIRContext *context, DialectAsmParser &parser, Type type) in mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp)

Yeah I had tried reproducing the FMF flag variant originally, but it was unable to build in this case because the linkage is an IntegerAttr and the new Linkage wrapper attribute wasn’t allowed to have an IntegerStorage as a parameter. The reason the FMF flags get away with it is because it is a bitfield.

FMF is in the exact same situation. There is some tech debt in the infrastructure (we have a new member starting soon, we may ask them to look into this!), because in ODS we can't decouple generating the Enum and all the stringification associated with it, from the Attribute generation. So there is LLVM::FMFAttr and LLVM::FastMathFlagsAttr that are both generated at the moment. The latter is just an IntegerAttribute indeed: class FastmathFlagsAttr : public ::mlir::IntegerAttr {.
The trick is to just not use this at all and use another attribute.

So this is what I did in D109209 ; with a twist compared to FMF: to avoid the name collision but still use Linkage for the enum and LinkageAttr for the attribute, I moved the generation of the enum inside a sub-namespace and I inlined only the enum-related symbol and not the generated attribute class that derive from IntegerAttr.

wsmoses updated this revision to Diff 370680.Fri, Sep 3, 4:03 PM

Rebase with LLVM linkage printer

This revision was landed with ongoing or failed builds.Fri, Sep 3, 5:42 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.Fri, Sep 3, 5:46 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
270

Do we have a verifier that forbid anything else than a LinkageAttr here and makes the cast always valid?

mehdi_amini added inline comments.Fri, Sep 10, 12:23 PM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
270

Ping here?

We do not but should. Apologies on the delay, will take a look at this week (was preparing for PACT and the start of the term).