[MLIR][LLVM] Refactor globals insertion point in import

[xlauko]

Unifies imports to use a single insertion point, globalInsertionOp, for global values.
Refactors insertion point setup into setGlobalInsertionPoint, which sets insertion point after globalInsertionOp or defaults to the start of the module if it is not set.

[llvmbot]

@llvm/pr-subscribers-mlir-llvm

@llvm/pr-subscribers-mlir

Author: Henrich Lauko (xlauko)

Changes

---

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

2 Files Affected:

• (modified) mlir/include/mlir/Target/LLVMIR/ModuleImport.h (+4)
• (modified) mlir/lib/Target/LLVMIR/ModuleImport.cpp (+13-15)

diff --git a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
index 4642d58760ca8..a44d2d3bd6ae5 100644
--- a/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
+++ b/mlir/include/mlir/Target/LLVMIR/ModuleImport.h
@@ -407,6 +407,10 @@ class ModuleImport {
   /// always requires a symbol name.
   FlatSymbolRefAttr
   getOrCreateNamelessSymbolName(llvm::GlobalVariable *globalVar);
+  /// Returns the global insertion point for the next global operation. If an
+  /// operation is set, the insertion point is placed after the specified
+  /// operation. Otherwise, it defaults to the start of the module.
+  OpBuilder::InsertionGuard setGlobalInsertionPoint(Operation *op);
 
   /// Builder pointing at where the next instruction should be generated.
   OpBuilder builder;
diff --git a/mlir/lib/Target/LLVMIR/ModuleImport.cpp b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
index fd0283b856b6b..8d053b7bfb814 100644
--- a/mlir/lib/Target/LLVMIR/ModuleImport.cpp
+++ b/mlir/lib/Target/LLVMIR/ModuleImport.cpp
@@ -962,13 +962,18 @@ ModuleImport::getOrCreateNamelessSymbolName(llvm::GlobalVariable *globalVar) {
   return symbolRef;
 }
 
-LogicalResult ModuleImport::convertAlias(llvm::GlobalAlias *alias) {
-  // Insert the global after the last one or at the start of the module.
+OpBuilder::InsertionGuard ModuleImport::setGlobalInsertionPoint(Operation *op) {
   OpBuilder::InsertionGuard guard(builder);
-  if (!aliasInsertionOp)
-    builder.setInsertionPointToStart(mlirModule.getBody());
+  if (op)
+    builder.setInsertionPointAfter(op);
   else
-    builder.setInsertionPointAfter(aliasInsertionOp);
+    builder.setInsertionPointToStart(mlirModule.getBody());
+  return guard;
+}
+
+LogicalResult ModuleImport::convertAlias(llvm::GlobalAlias *alias) {
+  // Insert the alias after the last one or at the start of the module.
+  OpBuilder::InsertionGuard guard = setGlobalInsertionPoint(aliasInsertionOp);
 
   Type type = convertType(alias->getValueType());
   AliasOp aliasOp = builder.create<AliasOp>(
@@ -996,11 +1001,7 @@ LogicalResult ModuleImport::convertAlias(llvm::GlobalAlias *alias) {
 
 LogicalResult ModuleImport::convertGlobal(llvm::GlobalVariable *globalVar) {
   // Insert the global after the last one or at the start of the module.
-  OpBuilder::InsertionGuard guard(builder);
-  if (!globalInsertionOp)
-    builder.setInsertionPointToStart(mlirModule.getBody());
-  else
-    builder.setInsertionPointAfter(globalInsertionOp);
+  OpBuilder::InsertionGuard guard = setGlobalInsertionPoint(globalInsertionOp);
 
   Attribute valueAttr;
   if (globalVar->hasInitializer())
@@ -1096,11 +1097,8 @@ ModuleImport::convertGlobalCtorsAndDtors(llvm::GlobalVariable *globalVar) {
     priorities.push_back(priority->getValue().getZExtValue());
   }
 
-  OpBuilder::InsertionGuard guard(builder);
-  if (!globalInsertionOp)
-    builder.setInsertionPointToStart(mlirModule.getBody());
-  else
-    builder.setInsertionPointAfter(globalInsertionOp);
+  // Insert the global after the last one or at the start of the module.
+  OpBuilder::InsertionGuard guard = setGlobalInsertionPoint(globalInsertionOp);
 
   if (globalVar->getName() == getGlobalCtorsVarName()) {
     globalInsertionOp = builder.create<LLVM::GlobalCtorsOp>(

[xlauko]

I am also not sure whether import needs to have separate insertion operation for globals and aliases (globalInsertionOp, aliasInsertionOp)?

FYI, I am working on GlobalIFunc import which uses similar insertion.

[Dinistro]

Suggested change

	/// operation is set, the insertion point is placed after the specified
	/// operation is provided, the insertion point is placed after the specified

Ultra nit: "set" can be a bit misleading/confusing

[Dinistro]

Considering that this is always called with globalInsertionOp, it might be acceptable to fold this into the function and avoid passing it at all?

[xlauko]

It is not. That was my question whether aliasInsertionOp is actually needed? I don't see a reason, but it was duplicated with aliases. I think globalInsertionOp might be enough. This will result to emit aliases after globals, which I guess desired anyway. Same applies for IFunc I wanted to add.

If this is true, I would inline that as well.

Though there is similar functionality with constantInsertionOp, which would require insertionBlock as argument as well.

[gysit]

I believe having just one globalInsertionOp makes sense. I even wonder why we introduced the insertions ops in the first place. It may also be possible to convert comdats/globals/aliases/functions etc in the desired order and insert them always at the end of the module. However, having one globalInsertionOp sounds like a good step forward.

[xlauko]

Except for an unexpected order in tests, everything seems to pass. I'll polish this further then. :)

[xlauko]

Should be fixed.

[gysit]

Nice!

LGTM