[MLIR][LLVM] Import LLVM target triple into MLIR LLVM Dialect

[ghehg]

It would be essential and useful info to have it in MLIR when we are doing optimizations at MLIR level using LLVM IR as input. Thought about making it part of ModuleImport::convertMetadata(), but decided not as this logic is its own simple thing. However, open to options like that.

[llvmbot]

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Guojin (ghehg)

Changes

It would be essential and useful info to have it in MLIR when we are doing optimizations at MLIR level using LLVM IR as input. Thought about making it part of [ModuleImport::convertMetadata()](https://github.com/llvm/llvm-project/blob/242aa8c743fe4344844753d8faf59744235319df/mlir/lib/Target/LLVMIR/ModuleImport.cpp#L597), but decided not as this logic is its own simple thing. However, open to options like that.

---

Full diff: https://github.com/llvm/llvm-project/pull/125084.diff

3 Files Affected:

• (modified) mlir/include/mlir/Target/LLVMIR/ModuleImport.h (+4)
• (modified) mlir/lib/Target/LLVMIR/ModuleImport.cpp (+6-1)
• (added) mlir/test/Target/LLVMIR/Import/target-triple.ll (+5)

diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
index 84aecbd4373e05..80ae4d679624c2 100644
--- a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
+++ b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
@@ -71,6 +71,10 @@ class ModuleImport {
   /// specification.
   LogicalResult convertDataLayout();
 
+  /// Converts target triple of the LLVM module to an MLIR target triple
+  /// specification.
+  void convertTargetTriple();
+
   /// Stores the mapping between an LLVM value and its MLIR counterpart.
   void mapValue(llvm::Value *llvm, Value mlir) { mapValue(llvm) = mlir; }
 
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index e23ffdedd9a60c..5ebde22cccbdf3 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -662,6 +662,11 @@ LogicalResult ModuleImport::convertDataLayout() {
   return success();
 }
 
+void ModuleImport::convertTargetTriple() {
+  mlirModule->setAttr(LLVM::LLVMDialect::getTargetTripleAttrName(),
+                      builder.getStringAttr(llvmModule->getTargetTriple()));
+}
+
 LogicalResult ModuleImport::convertFunctions() {
   for (llvm::Function &func : llvmModule->functions())
     if (failed(processFunction(&func)))
@@ -2451,6 +2456,6 @@ mlir::translateLLVMIRToModule(std::unique_ptr<llvm::Module> llvmModule,
     return {};
   if (failed(moduleImport.convertFunctions()))
     return {};
-
+  moduleImport.convertTargetTriple();
   return module;
 }
diff --git a/mlir/test/Target/LLVMIR/Import/target-triple.ll b/mlir/test/Target/LLVMIR/Import/target-triple.ll
new file mode 100644
index 00000000000000..a04b41a7aeb551
--- /dev/null
+++ b/mlir/test/Target/LLVMIR/Import/target-triple.ll
@@ -0,0 +1,5 @@
+; RUN: mlir-translate -import-llvm %s | FileCheck %s
+; CHECK: module attributes {
+; CHECK-SAME: llvm.target_triple = "aarch64-none-linux-android21"
+target triple = "aarch64-none-linux-android21"
+

[rengolin]

@rolfmorel @ftynse Interesting to know how this could tie up with DLTI.

[krzysz00]

If I had a vote, there should be a lossless [some sort of attribute] toMLIRDataLayout(const llvm::DataLayout& DL) - or perhaps [some sort of attribute] toMLIRDataLayout(StringRef llvmDataLayoutSpec) and its inverse.

But that's not this PR, I don't think.

I'll also toss out the radical position that a conversion from, say, memref to LLVM IR isn't valid unless there's a data layout spec present, since things like sizeof(!llvm.ptr) - or even sizeof(index) - are target-dependent and target-independent LLVM IR is an illusion.

[gysit]

Thanks, LGTM.

I think it is ok to have a separate function for the target triple import similar to the data layout import.

With regards to the data layout vs target triple discussion, I am in favor of keeping them separate to mirror LLVM IR. Usually, LLVM dialect tries to follow LLVM IR as closely as possible. However, I agree that there is room for improving the representation of the target triple in LLVM dialect (currently it is a string attribute).

[gysit]

If I had a vote, there should be a lossless [some sort of attribute] toMLIRDataLayout(const llvm::DataLayout& DL) - or perhaps [some sort of attribute] toMLIRDataLayout(StringRef llvmDataLayoutSpec) and its inverse.

There are such functions +- in the ModuleImport / ModuleTranslation. In particular, the import from LLVM IR uses the DataLayoutImporter class. The process is not completely lossless though since we do not model all fields in MLIR.

[krzysz00]

... that's what I get for misreading the title of the PR

Yeah, importing the target triple is a good idea but I don't think it affects DLTI much at all - at least at this point

[krzysz00]

@gysit Re not modeling all fields ... what're we missing? I suspect vector alignments and ... probably some of the address space metadata?

(My long term vision here is that, when going to LLVM, you'd have a, say, #rocdl.target<..., [otherDataLayoutEntries]> or #x86.target<..., [otherDataLayoutEntries]>` attribute sitting in scope so that all the things that're known from the LLVM data layout would be implicitly populated without having to write all of them out.

[gysit]

@gysit Re not modeling all fields ... what're we missing? I suspect vector alignments and ... probably some of the address space metadata?

Yes vector and I believe aggregate and function pointer alignment as well. Plus some exotic name mangling flag etc.

(My long term vision here is that, when going to LLVM, you'd have a, say, #rocdl.target<..., [otherDataLayoutEntries]> or #x86.target<..., [otherDataLayoutEntries]>` attribute sitting in scope so that all the things that're known from the LLVM data layout would be implicitly populated without having to write all of them out.

The main goal would be that the default data layout is chosen based on the target? That sounds like a good option to me.

PS I will land this commit later today should there be no more comments.

[rengolin]

Yeah, importing the target triple is a good idea but I don't think it affects DLTI much at all - at least at this point

Sorry, was just tagging folks on the DLTI design discussion.

I think DLTI could be generating those attributes, too, not just when it's imported from LLVM. My thinking is to have a coherent whole-program target description story, from front-end (be it pytorch or clang), MLIR stack (via DLTI) and LLVM (via triples and DL).