[Mlir-commits] [mlir] [MLIR][LLVM] Fix llvm.mlir.global mismatching print and parser order (PR #138986)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir

Author: Bruno Cardoso Lopes (bcardosolopes)

<details>
<summary>Changes</summary>

`GlobalOp` was parsing `thread_local` after `unnamed_addr`, but printing in the reverse order.

While here, make `AliasOp` match the same behavior and share common parts of global and alias printing.

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


3 Files Affected:

- (modified) mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp (+15-22) 
- (modified) mlir/test/Dialect/LLVMIR/alias.mlir (+2-2) 
- (modified) mlir/test/Dialect/LLVMIR/roundtrip.mlir (+3) 


``````````diff
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
index e17b9fd6eb8d3..fc74e3d10a719 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
@@ -2215,18 +2215,23 @@ void GlobalOp::build(OpBuilder &builder, OperationState &result, Type type,
   result.addRegion();
 }
 
-void GlobalOp::print(OpAsmPrinter &p) {
-  p << ' ' << stringifyLinkage(getLinkage()) << ' ';
-  StringRef visibility = stringifyVisibility(getVisibility_());
+template <typename OpType>
+static void printCommonGlobalAndAlias(OpAsmPrinter &p, OpType op) {
+  p << ' ' << stringifyLinkage(op.getLinkage()) << ' ';
+  StringRef visibility = stringifyVisibility(op.getVisibility_());
   if (!visibility.empty())
     p << visibility << ' ';
-  if (getThreadLocal_())
+  if (op.getThreadLocal_())
     p << "thread_local ";
-  if (auto unnamedAddr = getUnnamedAddr()) {
+  if (auto unnamedAddr = op.getUnnamedAddr()) {
     StringRef str = stringifyUnnamedAddr(*unnamedAddr);
     if (!str.empty())
       p << str << ' ';
   }
+}
+
+void GlobalOp::print(OpAsmPrinter &p) {
+  printCommonGlobalAndAlias<GlobalOp>(p, *this);
   if (getConstant())
     p << "constant ";
   p.printSymbolName(getSymName());
@@ -2309,16 +2314,16 @@ static ParseResult parseCommonGlobalAndAlias(OpAsmParser &parser,
                           parseOptionalLLVMKeyword<LLVM::Visibility, int64_t>(
                               parser, result, LLVM::Visibility::Default)));
 
+  if (succeeded(parser.parseOptionalKeyword("thread_local")))
+    result.addAttribute(OpType::getThreadLocal_AttrName(result.name),
+                        parser.getBuilder().getUnitAttr());
+
   // Parse optional UnnamedAddr, default to None.
   result.addAttribute(OpType::getUnnamedAddrAttrName(result.name),
                       parser.getBuilder().getI64IntegerAttr(
                           parseOptionalLLVMKeyword<UnnamedAddr, int64_t>(
                               parser, result, LLVM::UnnamedAddr::None)));
 
-  if (succeeded(parser.parseOptionalKeyword("thread_local")))
-    result.addAttribute(OpType::getThreadLocal_AttrName(result.name),
-                        parser.getBuilder().getUnitAttr());
-
   return success();
 }
 
@@ -2581,19 +2586,7 @@ void AliasOp::build(OpBuilder &builder, OperationState &result, Type type,
 }
 
 void AliasOp::print(OpAsmPrinter &p) {
-  p << ' ' << stringifyLinkage(getLinkage()) << ' ';
-  StringRef visibility = stringifyVisibility(getVisibility_());
-  if (!visibility.empty())
-    p << visibility << ' ';
-
-  if (std::optional<mlir::LLVM::UnnamedAddr> unnamedAddr = getUnnamedAddr()) {
-    StringRef str = stringifyUnnamedAddr(*unnamedAddr);
-    if (!str.empty())
-      p << str << ' ';
-  }
-
-  if (getThreadLocal_())
-    p << "thread_local ";
+  printCommonGlobalAndAlias<AliasOp>(p, *this);
 
   p.printSymbolName(getSymName());
   p.printOptionalAttrDict((*this)->getAttrs(),
diff --git a/mlir/test/Dialect/LLVMIR/alias.mlir b/mlir/test/Dialect/LLVMIR/alias.mlir
index efba248534862..7ce54f35a5557 100644
--- a/mlir/test/Dialect/LLVMIR/alias.mlir
+++ b/mlir/test/Dialect/LLVMIR/alias.mlir
@@ -130,12 +130,12 @@ llvm.mlir.global internal constant @g3() : !llvm.ptr {
 
 llvm.mlir.global private @g30(0 : i32) {dso_local} : i32
 
-llvm.mlir.alias private unnamed_addr thread_local @a30 {dso_local} : i32 {
+llvm.mlir.alias private thread_local unnamed_addr @a30 {dso_local} : i32 {
   %0 = llvm.mlir.addressof @g30 : !llvm.ptr
   llvm.return %0 : !llvm.ptr
 }
 
-// CHECK: llvm.mlir.alias private unnamed_addr thread_local @a30 {dso_local} : i32 {
+// CHECK: llvm.mlir.alias private thread_local unnamed_addr @a30 {dso_local} : i32 {
 // CHECK:   %0 = llvm.mlir.addressof @g30 : !llvm.ptr
 // CHECK:   llvm.return %0 : !llvm.ptr
 // CHECK: }
diff --git a/mlir/test/Dialect/LLVMIR/roundtrip.mlir b/mlir/test/Dialect/LLVMIR/roundtrip.mlir
index 1fd24f3f58e44..2e6acc13d1627 100644
--- a/mlir/test/Dialect/LLVMIR/roundtrip.mlir
+++ b/mlir/test/Dialect/LLVMIR/roundtrip.mlir
@@ -1042,3 +1042,6 @@ llvm.func @llvm.aarch64.neon.st3.v8i8.p0(vector<8xi8>, vector<8xi8>, vector<8xi8
 
 // CHECK-LABEL: llvm.func @callintrin_voidret
 // CHECK-NEXT: llvm.call_intrinsic "llvm.aarch64.neon.st3.v8i8.p0"(%arg0, %arg1, %arg2, %arg3) : (vector<8xi8>, vector<8xi8>, vector<8xi8>, !llvm.ptr) -> ()
+
+llvm.mlir.global internal thread_local unnamed_addr @myglobal(-1 : i32) {addr_space = 0 : i32, alignment = 4 : i64, dso_local} : i32
+// CHECK: llvm.mlir.global internal thread_local unnamed_addr @myglobal(-1 : i32) {addr_space = 0 : i32, alignment = 4 : i64, dso_local} : i32

``````````

</details>


https://github.com/llvm/llvm-project/pull/138986