Page MenuHomePhabricator

[mlir] replace 'emit_c_wrappers' func->llvm conversion option with a pass
ClosedPublic

Authored by ftynse on Jun 16 2022, 3:54 AM.

Details

Summary

The 'emit_c_wrappers' option in the FuncToLLVM conversion requests C interface
wrappers to be emitted for every builtin function in the module. While this has
been useful to bootstrap the interface, it is problematic in the longer term as
it may unintentionally affect the functions that should retain their existing
interface, e.g., libm functions obtained by lowering math operations (see
D126964 for an example). Since D77314, we have a finer-grain control over
interface generation via an attribute that avoids the problem entirely. Remove
the 'emit_c_wrappers' option. Introduce the '-llvm-request-c-wrappers' pass
that can be run in any pipeline that needs blanket emission of functions to
annotate all builtin functions with the attribute before performing the usual
lowering that accounts for the attribute.

Diff Detail

Event Timeline

ftynse created this revision.Jun 16 2022, 3:54 AM
ftynse requested review of this revision.Jun 16 2022, 3:54 AM
zero9178 added a subscriber: zero9178.EditedJun 16 2022, 5:01 AM

Very big fan of this change. That option in particular is very specific to a single scenario (memrefs as arguments), and being able to move it out of a generic options class is a lot cleaner.

The layering here is a bit odd to me however. The way llvm.emit_c_interface is currently implemented, is only applicable to func.func to llvm.func conversions (which is fair), yet the convenience pass to add the llvm.emit_c_wrapper attribute to all functions lives in LLVM Dialect transforms and depends on func.func.
The way I see it either:

  • llvm.emit_c_interface is meant as a general purpose indicator that any dialect may use during type lowering. In that case the pass should probably be a generic operation pass and not only apply to mlir::func::FuncOp. This would also drop the link dependency on MLIRFuncDialect from LLVMIRTransforms
  • llvm.emit_c_interface shouldn't be part of the LLVM dialect but rather of the func dialect. There is currently no code that actually checks llvm.emit_c_interface except in FuncToLLVM.cpp

Very big fan of this change. That option in particular is very specific to a single scenario (memrefs as arguments), and being able to move it out of a generic options class is a lot cleaner.

The layering here is a bit odd to me however. The way llvm.emit_c_interface is currently implemented, is only applicable to func.func to llvm.func conversions (which is fair), yet the convenience pass to add the llvm.emit_c_wrapper attribute to all functions lives in LLVM Dialect transforms and depends on func.func.
The way I see it either:

  • llvm.emit_c_interface is meant as a general purpose indicator that any dialect may use during type lowering. In that case the pass should probably be a generic operation pass and not only apply to mlir::func::FuncOp. This would also drop the link dependency on MLIRFuncDialect from LLVMIRTransforms
  • llvm.emit_c_interface shouldn't be part of the LLVM dialect but rather of the func dialect. There is currently no code that actually checks llvm.emit_c_interface except in FuncToLLVM.cpp

My two cents on this: I'm not too fond of options one as I don't see many other use cases for the attribute. Option two may have the problem that any pass before conversion to LLVM can or cannot discard the attribute. I believe that's why we want to do that right before conversion to LLVM.

Very big fan of this change. That option in particular is very specific to a single scenario (memrefs as arguments), and being able to move it out of a generic options class is a lot cleaner.

The layering here is a bit odd to me however. The way llvm.emit_c_interface is currently implemented, is only applicable to func.func to llvm.func conversions (which is fair), yet the convenience pass to add the llvm.emit_c_wrapper attribute to all functions lives in LLVM Dialect transforms and depends on func.func.
The way I see it either:

  • llvm.emit_c_interface is meant as a general purpose indicator that any dialect may use during type lowering. In that case the pass should probably be a generic operation pass and not only apply to mlir::func::FuncOp. This would also drop the link dependency on MLIRFuncDialect from LLVMIRTransforms
  • llvm.emit_c_interface shouldn't be part of the LLVM dialect but rather of the func dialect. There is currently no code that actually checks llvm.emit_c_interface except in FuncToLLVM.cpp

My two cents on this: I'm not too fond of options one as I don't see many other use cases for the attribute. Option two may have the problem that any pass before conversion to LLVM can or cannot discard the attribute. I believe that's why we want to do that right before conversion to LLVM.

Regarding option one: Other dialects with a custom function op might want to have the same behaviour when lowering to LLVM in the future. This option is also a tiny change from the current state of the patch.

Regarding option two: Passes can discard the attribute regardless of whether it is part of the LLVM dialect of the func dialect. The second option would not change anything in that regard except that we'd probably want the pass to live in the Func Dialect instead of the LLVM Dialect. It being run before the FuncToLLVM pass would remain a requirement.

Very big fan of this change. That option in particular is very specific to a single scenario (memrefs as arguments), and being able to move it out of a generic options class is a lot cleaner.

The layering here is a bit odd to me however. The way llvm.emit_c_interface is currently implemented, is only applicable to func.func to llvm.func conversions (which is fair), yet the convenience pass to add the llvm.emit_c_wrapper attribute to all functions lives in LLVM Dialect transforms and depends on func.func.
The way I see it either:

  • llvm.emit_c_interface is meant as a general purpose indicator that any dialect may use during type lowering. In that case the pass should probably be a generic operation pass and not only apply to mlir::func::FuncOp. This would also drop the link dependency on MLIRFuncDialect from LLVMIRTransforms
  • llvm.emit_c_interface shouldn't be part of the LLVM dialect but rather of the func dialect. There is currently no code that actually checks llvm.emit_c_interface except in FuncToLLVM.cpp

My two cents on this: I'm not too fond of options one as I don't see many other use cases for the attribute. Option two may have the problem that any pass before conversion to LLVM can or cannot discard the attribute. I believe that's why we want to do that right before conversion to LLVM.

Regarding option one: Other dialects with a custom function op might want to have the same behaviour when lowering to LLVM in the future. This option is also a tiny change from the current state of the patch.

Right my bad; I didn't understand from your previous comment how you want to scope the attribute if we move to a generic operation pass. This attribute is fairly specific and should apply only for function-like operations. So if this is what you have in mind, I agree. If we move to an operation pass, I suggest keeping the attribute in the LLVM dialect.

Regarding option two: Passes can discard the attribute regardless of whether it is part of the LLVM dialect of the func dialect. The second option would not change anything in that regard except that we'd probably want the pass to live in the Func Dialect instead of the LLVM Dialect. It being run before the FuncToLLVM pass would remain a requirement.

I went for the least worst solution here. FuncTransforms library already depends on "higher level" dialects (SCF, MemRef and co.), adding the "lower level" dialect sounds undesirable. LLVMTransforms, on the other hand, can have a dependency on the "higher level" Func dialect and be consistent. (Note that there is no dependency between dialects themselves.)

The attribute is only relevant for the func->llvm conversion, so the independent place for it would be somewhere in lib/Conversions/FuncToLLVM/AdditionalPasses, but that would be a significant architectural change, even though with a small amount of code. Making it into an operation pass will allow the attribute to be attached to any operation, where it doesn't make sense and will be confusing. As for some hypothetical other dialect that may want this, I would rather not overgeneralize prematurely. When and if this dialect appears, we can reconsider. FWIW, the only reason the pass is introduced is some backwards compatibility for downstreams that may depend on this behavior to be configurable from the textual pass pipeline. Otherwise, the code to add the attribute to all functions takes three lines, it can be duplicated at will.

chelini accepted this revision.Jun 16 2022, 9:33 AM

It makes sense to me.

This revision is now accepted and ready to land.Jun 16 2022, 9:33 AM
chelini added inline comments.Jun 16 2022, 9:37 AM
mlir/include/mlir/Dialect/LLVMIR/Transforms/Passes.td
24

requires -> instructs

I went for the least worst solution here. FuncTransforms library already depends on "higher level" dialects (SCF, MemRef and co.), adding the "lower level" dialect sounds undesirable. LLVMTransforms, on the other hand, can have a dependency on the "higher level" Func dialect and be consistent. (Note that there is no dependency between dialects themselves.)

The attribute is only relevant for the func->llvm conversion, so the independent place for it would be somewhere in lib/Conversions/FuncToLLVM/AdditionalPasses, but that would be a significant architectural change, even though with a small amount of code. Making it into an operation pass will allow the attribute to be attached to any operation, where it doesn't make sense and will be confusing. As for some hypothetical other dialect that may want this, I would rather not overgeneralize prematurely. When and if this dialect appears, we can reconsider. FWIW, the only reason the pass is introduced is some backwards compatibility for downstreams that may depend on this behavior to be configurable from the textual pass pipeline. Otherwise, the code to add the attribute to all functions takes three lines, it can be duplicated at will.

I see thank you

This revision was landed with ongoing or failed builds.Jun 17 2022, 2:10 AM
This revision was automatically updated to reflect the committed changes.
ftynse marked an inline comment as done.