This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Support lowering of dialect attributes attached to top-level modules
ClosedPublic

Authored by skatrak on Mar 13 2023, 6:21 AM.

Details

Summary

This patch provides an alternative approach to D144883, with the goal of
supporting the processing of dialect attributes attached to top-level module-
type operations during MLIR-to-LLVMIR lowering.

Based on reviewer comments, this approach modifies the
mlir::translateModuleToLLVMIR() function to call
ModuleTranslation::convertOperation() on the top-level operation, after its
body has been lowered. This, in turn, will get the
LLVMTranslationDialectInterface object associated to that operation's dialect
before trying to use it for lowering prior to processing dialect attributes
attached to the operation.

Since there are no LLVMTranslationDialectInterfaces for the builtin and GPU
dialects, which define their own module-type operations, this patch also adds
and registers them. Their purpose is to just succeed when processing module
operations, to allow the lowering process to continue and to prevent the
introduction of failures related to not finding such interfaces.

The mentioned interface is added to the builtin dialect, differently to every
other dialect, on creation. Alternatively, it would require substantial changes
to be registered before any lowering to LLVM IR can be done.

Diff Detail

Event Timeline

skatrak created this revision.Mar 13 2023, 6:21 AM
skatrak requested review of this revision.Mar 13 2023, 6:21 AM
mlir/lib/IR/BuiltinDialect.cpp
115 ↗(On Diff #504622)

I think this is best done in the Flang flow, so as to not make the builtin dialect llvm translation aware. I believe this is what @ftynse also recommended in https://reviews.llvm.org/D144883. We have done this previously for the OpenMP_PointerLikeType interface in https://github.com/llvm/llvm-project/blob/main/flang/include/flang/Tools/PointerModels.h.

We can also use the external model mechanism https://mlir.llvm.org/docs/Interfaces/#external-models-for-attribute-operation-and-type-interfaces so we don't need to make the builtin dialect aware of the LLVM translation interface.
mlir/lib/Target/LLVMIR/Dialect/GPU/GPUToLLVMIRTranslation.cpp
2

Nit: OpenMP -> GPU.

10–11

Nit: OpenMP -> GPU.

skatrak added inline comments.Mar 14 2023, 4:06 AM
mlir/lib/IR/BuiltinDialect.cpp
115 ↗(On Diff #504622)

Thank you Kiran for the feedback and for pointing me to an example, as I wasn't sure exactly what that approach would look like in this case.

It makes sense that we wouldn't want the builtin dialect itself to know about an interface for translating it to LLVM IR, but it looks like we can't quite keep it localized to Flang though. The reason is that, after this change, any code intending to lower to LLVM IR must register a LLVMTranslationDialectInterface for at least the builtin dialect and the LLVM dialect. Otherwise, the call to translator.convertOperation(*module, llvmBuilder) in ModuleTranslation.cpp would fail for the builtin.module operation, aborting the translation process. Basically every MLIR lit test outside of Flang that invokes the translation into LLVM IR would fail because of this.

So, I'm currently thinking about incorporating feedback so far into this alternative solution, but I'd like to check whether this is an acceptable way to go:

  1. Create a ModuleLikeOp interface, or maybe call it SkippableLLVMIRTranslationOp (listening for any suggestions on a name here), without any methods defined to be used as a tag.
  2. Attach that interface to mlir::ModuleOp in mlir::registerLLVMDialectTranslation(), and attach the LLVMTranslationDialectInterface to the builtin dialect.
  3. Modify mlir::registerGPUDialectTranslation() to do the same as steps 2 for the mlir::gpu::GPUModuleOp and mlir::gpu::GPUDialect, removing the GPUDialectLLVMIRTranslationInterface I created in this patch.
  4. Modify LLVMTranslationDialectInterface::convertOperation() to succeed by default if the op has the ModuleLikeOp interface attached.

This way, by registering the interfaces in mlir::registerLLVMDialectTranslation(), which is called by every spot that needs lowering to LLVM IR, we avoid errors due to not finding these lowering interfaces for the builtin.module without making the dialect itself aware of it. Also, using the ModuleLikeOp interface or similar, we can skip defining an LLVM IR translation interface for dialects such as the GPU one that define their own module operation but are not (at least currently) intended to be lowered to LLVM IR directly. These are two separate ideas, so it could also be that I could just do one of them as well.

The alternative I see to step 2, to avoid mixing dialects in a spot where it might be better not to, would be to create an mlir::registerBuiltinDialectTranslation() that would need to be called in every place in which mlir::registerLLVMDialectTranslation() is called, which I see as very invasive. Maybe the compromise here would be to consolidate both calls into something like mlir::registerDefaultDialectsToLLVMTranslation(), and replace all calls to mlir::registerLLVMDialectTranslation() for it.

ftynse requested changes to this revision.Mar 14 2023, 11:17 AM
ftynse added inline comments.
mlir/lib/IR/BuiltinDialect.cpp
115 ↗(On Diff #504622)

This looks overly complicated IMO. And we still don't want to be touching code in lib/IR for this.

Why can't we have the interface for the Builtin dialect be implemented the same way as any other dialect? Yes, the client will have to call the registration function, but it shouldn't be an issue given how many things they already regsiter.

This revision now requires changes to proceed.Mar 14 2023, 11:17 AM
skatrak updated this revision to Diff 506571.Mar 20 2023, 7:20 AM

Implement translation from builtin dialect to LLVM IR like for all other dialects

Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: jplehr. · View Herald Transcript
skatrak updated this revision to Diff 506573.Mar 20 2023, 7:24 AM

Fix error in comment

skatrak marked 2 inline comments as done.Mar 20 2023, 7:32 AM
skatrak added inline comments.
mlir/lib/IR/BuiltinDialect.cpp
115 ↗(On Diff #504622)

I just made the changes to have the interface for the Builtin dialect be implemented like for the others, as well as registering it by all potential clients to avoid errors during translation. The only reason to avoid doing it this way was that the changes following this approach seem more substantial and error-prone.

ftynse accepted this revision.Mar 20 2023, 9:46 AM

Nice, thanks!

mlir/include/mlir/Target/LLVMIR/Dialect/Builtin/BuiltinToLLVMIRTranslation.h
1–2

Nit: this should be one line.

mlir/include/mlir/Target/LLVMIR/Dialect/GPU/GPUToLLVMIRTranslation.h
1–2

Ditto.

mlir/lib/Target/LLVMIR/Dialect/Builtin/BuiltinToLLVMIRTranslation.cpp
30

Nit: return success(isa<ModuleOp>(op)) is more concise.

This revision is now accepted and ready to land.Mar 20 2023, 9:46 AM
skatrak updated this revision to Diff 506916.Mar 21 2023, 4:45 AM

Address last reviewer's comments

skatrak marked 3 inline comments as done.Mar 21 2023, 5:00 AM

Thank you for all the suggestions!