[MLIR] Move builtin.module LLVM IR translation to before nested operations

[skatrak]

Summary

This patch moves the call for translating an MLIR module to LLVM IR to the beginning of the translation process. This enables the use of dialect attributes attached to builtin.module operations and the amendOperation() flow to initialize dialect-specific global configuration before translating the contents of the module.

Currently, this patch does not impact the generated IR on its own.

[skatrak]

Ping for review! Thank you

[kiranchandramohan]

LGTM. Please wait a couple of days to see if @ftynse @gysit @Dinistro @zero9178 have any comments here.

[zero9178]

Makes sense to me. IIRC the dialect conversion framework also does things in pre-order. This now nicely turns the translation into "module -> top-level-ops -> function-bodies" order.

[gysit]

The change looks good to me!

Would it be possible to add a test, e.g. by implementing the translation interface for the test dialect and then attaching a test dialect attribute to the module?

There have been other revisions that modified the translation order and it may be a good idea to have some testing to ensure follow up commits do not break previous assumptions.

[skatrak]

Add testing infrastructure to allow translating operations from the Test dialect to LLVM IR, and add a simple test to make sure that module attributes are translated before the module body.

[skatrak]

Thank you all for having a look and for your suggestions! They should be addressed now, but I'd like to request another quick review, since I had to make significant additions in order to test this change using the Test dialect.

[zero9178]

mlir/test/lib/Dialect/Test/CMakeLists.txt

87–88 ↗

(On Diff #555360)

Having the TestDialect depend on these is problematic I believe as these depend on LLVM proper so will pull in LLVM IR and such as link constructs.
You'd likely see this if building with shared libs.

I think the best way to implement this is to make a separate add_mlir_translation_library just for TestToLLVMIRTranslation that depends on TestDialect, builds`TestToLLVMIRTranslation.cpp` and make mlir-translate link against that if MLIR_INCLUDE_TESTS is true in CMake.

[gysit]

Thanks for adding the test!

Looks great modulo the packaging in a separate library suggested by @zero9178.

[skatrak]

Split translation of Test dialect to LLVM IR into its own MLIR translation library.

[skatrak]

mlir/test/lib/Dialect/Test/CMakeLists.txt

87–88 ↗

(On Diff #555360)

Thank you for the pointers, I just finished making these changes. In the end, I was able to keep the CMakeLists.txt for mlir-translate unmodified because the add_mlir_translation_library call registers the library into the MLIR_TRANSLATION_LIBS variable that's already used by mlir-translate to find libraries to link to.

[zero9178]

LGTM

[gysit]

Thank you for the pointers, I just finished making these changes. In the end, I was able to keep the CMakeLists.txt for mlir-translate unmodified because the add_mlir_translation_library call registers the library into the MLIR_TRANSLATION_LIBS variable that's already used by mlir-translate to find libraries to link to.

Nice!

LGTM

[skatrak]

This seems to have broken post-commit builds (1, 2), so I reverted the change until I can take a look at it next week. Sorry for the noise!

[zero9178]

In D158278#4634775, @skatrak wrote:

This seems to have broken post-commit builds (1, 2), so I reverted the change until I can take a look at it next week. Sorry for the noise!

No worries! Reverting is the right thing to do :) I believe you might have to use EXCLUDE_FROM_LIBMLIR in your add_mlir_translation_library, just like the test dialect does above. The windows build failure seems unrelated to your change.

[skatrak]

In D158278#4634799, @zero9178 wrote:

In D158278#4634775, @skatrak wrote:

This seems to have broken post-commit builds (1, 2), so I reverted the change until I can take a look at it next week. Sorry for the noise!

No worries! Reverting is the right thing to do :) I believe you might have to use EXCLUDE_FROM_LIBMLIR in your add_mlir_translation_library, just like the test dialect does above. The windows build failure seems unrelated to your change.

Thank you, that does seem to be the solution to the problem. I should have read your comment before spending the time reaching the same conclusion! I'll re-land this shortly.

[mehdi_amini]

Please always document the commit message when doing reverts including why a revert is happening.

Also instead when reverting reverts, include the original commit message (and mention "re-land commit abc12345" in the description instead), otherwise git log / git blame is harder for everyone.

[skatrak]

In D158278#4642166, @mehdi_amini wrote:

Please always document the commit message when doing reverts including why a revert is happening.

Also instead when reverting reverts, include the original commit message (and mention "re-land commit abc12345" in the description instead), otherwise git log / git blame is harder for everyone.

Yes, sorry about that. I did get similar feedback right after I re-landed the patch, I'll keep this in mind for next time.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

mlir::translateModuleToLLVMIR(Operation *module, llvm::LLVMContext &llvmContext,
   std::unique_ptr<llvm::Module> llvmModule =
       prepareLLVMModule(module, llvmContext, name);
   if (!llvmModule)
     return nullptr;
 
   LLVM::ensureDistinctSuccessors(module);
 
   ModuleTranslation translator(module, std::move(llvmModule));
+  llvm::IRBuilder<> llvmBuilder(llvmContext);
+
+  // Convert module before functions and operations inside, so dialect
+  // attributes can be used to change dialect-specific global configurations via
+  // `amendOperation()`. These configurations can then influence the translation
+  // of operations afterwards.
+  if (failed(translator.convertOperation(*module, llvmBuilder)))
+    return nullptr;
+
   if (failed(translator.convertComdats()))
     return nullptr;
   if (failed(translator.convertFunctionSignatures()))
     return nullptr;
   if (failed(translator.convertGlobals()))
     return nullptr;
   if (failed(translator.createAliasScopeMetadata()))
     return nullptr;
   if (failed(translator.createTBAAMetadata()))
     return nullptr;
 
   // Convert other top-level operations if possible.
-  llvm::IRBuilder<> llvmBuilder(llvmContext);
   for (Operation &o : getModuleBody(module).getOperations()) {
     if (!isa<LLVM::LLVMFuncOp, LLVM::GlobalOp, LLVM::GlobalCtorsOp,
              LLVM::GlobalDtorsOp, LLVM::ComdatOp>(&o) &&
         !o.hasTrait<OpTrait::IsTerminator>() &&
         failed(translator.convertOperation(o, llvmBuilder))) {
       return nullptr;
     }
   }
 
   // Operations in function bodies with symbolic references must be converted
   // after the top-level operations they refer to are declared, so we do it
   // last.
   if (failed(translator.convertFunctions()))
     return nullptr;
 
-  // Convert module itself.
-  if (failed(translator.convertOperation(*module, llvmBuilder)))
-    return nullptr;
-
   if (llvm::verifyModule(*translator.llvmModule, &llvm::errs()))
     return nullptr;
 
   return std::move(translator.llvmModule);
 }