[mlir][llvm] Update insertion point handling in LLVM import.

[gysit]

Summary

Insert constants and globals in order by maintaining the position
of the constant and global inserted last. Update the tests
to reflect the updated insertion order. Also make sure functions
are always inserted at the end of the module instead of at
the second last position and delete a spurious function in
the intrinsic.ll that seems to exist to avoid the first
function under test ends up at the end of the module.

[ftynse]

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

423

Naming nit: this returns a block rather than the insertion point, which is a combination a of a block and an iteration in it.

Conceptual nit: by doing this, we will always insert constants in the inverse order of their encounter. This makes the IR surprising to read, but isn't really a functional change. Some ideas for handling it in the future: can't we maintain the Operation * of the last inserted constant and use that to set the insertion point after it?

433–436

Similar as above: can't we just maintain the Operation * of the first function and set the insertion point before it for globals to avoid iterating over the module?

[gysit]

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

423

I agree the inverse order is problematic and will try to implement a variant with Operation *. Alternatively, we may also have multiple builders?

[ftynse]

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

423

We may have multiple builders as long as we carefully propagate their extra state such as listeners, although it may not be a concern right now. IMO just setting the insertion point is simple and more future-proof.

[gysit]

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

423

Sounds good!

[gysit]

Address comments by tracking the insertion points using Operation* / Block*.

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

private:
   Type getStdTypeForAttr(Type type);
   /// Return `value` as an attribute to attach to a GlobalOp.
   Attribute getConstantAsAttr(llvm::Constant *value);
   /// Return `constant` as an MLIR Value. This could either be a ConstantOp, or
   /// an expanded sequence of ops in the current function's entry block (for
   /// ConstantExprs or ConstantGEPs).
   Value processConstant(llvm::Constant *constant);
 
+  /// Returns the entry block for the current insertion point.
+  Block *getConstantInsertionPoint() {
+    Region *region = nullptr;
+    if (builder.getInsertionBlock())
+      region = builder.getInsertionBlock()->getParent();
+    assert(region && "expected builder to insert into a region");
+    return &region->front();
+  }
+  /// Returns an iterator pointing to the first function or to the end of the
+  /// module if there is none.
+  Block::iterator getGlobalInsertionPoint() {
+    auto ops = module.getBody()->getOps<LLVMFuncOp>();
+    if (!ops.empty())
+      return ops.begin();
+    return module.getBody()->end();
+  }
+  /// Returns an iterator pointing to the end of the module.
+  Block::iterator getFunctionInsertionPoint() {
+    return module.getBody()->end();
+  }
+
   /// Builder pointing at where the next Instruction should be generated.
   OpBuilder builder;
   /// The current context.
   MLIRContext *context;
   /// The current module being created.
   ModuleOp module;
-  /// The entry block of the current function being processed.
-  Block *currentEntryBlock = nullptr;
-
-  /// Globals are inserted before the first function, if any.
-  Block::iterator getGlobalInsertPt() {
-    Block::iterator it = module.getBody()->begin();
-    Block::iterator endIt = module.getBody()->end();
-    while (it != endIt && !isa<LLVMFuncOp>(it))
-      ++it;
-    return it;
-  }
-
-  /// Functions are always inserted before the module terminator.
-  Block::iterator getFuncInsertPt() {
-    return std::prev(module.getBody()->end());
-  }
 
   /// Function-local mapping between original and imported block.
   DenseMap<llvm::BasicBlock *, Block *> blockMapping;
   /// Function-local mapping between original and imported values.
   DenseMap<llvm::Value *, Value> valueMapping;
   /// Uniquing map of GlobalVariables.
   DenseMap<llvm::GlobalVariable *, GlobalOp> globals;
   /// The stateful type translator (contains named structs).
Attribute Importer::getConstantAsAttr(llvm::Constant *value) {
   return nullptr;
 }
 
 GlobalOp Importer::processGlobal(llvm::GlobalVariable *gv) {
   auto it = globals.find(gv);
   if (it != globals.end())
     return it->second;
 
-  OpBuilder b(module.getBody(), getGlobalInsertPt());
+  // Insert the global at the beginning of the module before the first function.
+  OpBuilder::InsertionGuard guard(builder);
+  builder.setInsertionPoint(module.getBody(), getGlobalInsertionPoint());
+
   Attribute valueAttr;
   if (gv->hasInitializer())
     valueAttr = getConstantAsAttr(gv->getInitializer());
   Type type = convertType(gv->getValueType());
 
   uint64_t alignment = 0;
   llvm::MaybeAlign maybeAlign = gv->getAlign();
   if (maybeAlign.has_value()) {
     llvm::Align align = maybeAlign.value();
     alignment = align.value();
   }
 
-  GlobalOp op = b.create<GlobalOp>(
+  GlobalOp op = builder.create<GlobalOp>(
       UnknownLoc::get(context), type, gv->isConstant(),
       convertLinkageFromLLVM(gv->getLinkage()), gv->getName(), valueAttr,
       alignment, /*addr_space=*/gv->getAddressSpace(),
       /*dso_local=*/gv->isDSOLocal(), /*thread_local=*/gv->isThreadLocal());
 
   if (gv->hasInitializer() && !valueAttr) {
-    Region &r = op.getInitializerRegion();
-    currentEntryBlock = b.createBlock(&r);
-    b.setInsertionPoint(currentEntryBlock, currentEntryBlock->begin());
-    Value v = processConstant(gv->getInitializer());
-    if (!v)
+    Block *block = builder.createBlock(&op.getInitializerRegion());
+    builder.setInsertionPointToStart(block);
+    Value value = processConstant(gv->getInitializer());
+    if (!value)
       return nullptr;
-    b.create<ReturnOp>(op.getLoc(), ArrayRef<Value>({v}));
+    builder.create<ReturnOp>(op.getLoc(), ArrayRef<Value>({value}));
   }
   if (gv->hasAtLeastLocalUnnamedAddr())
     op.setUnnamedAddr(convertUnnamedAddrFromLLVM(gv->getUnnamedAddr()));
   if (gv->hasSection())
     op.setSection(gv->getSection());
 
   return globals[gv] = op;
 }
 
 Value Importer::processConstant(llvm::Constant *constant) {
-  OpBuilder bEntry(currentEntryBlock, currentEntryBlock->begin());
+  // Insert the constant at the beginning of the region entry block.
+  OpBuilder::InsertionGuard guard(builder);
+  builder.setInsertionPointToStart(getConstantInsertionPoint());
+
   if (Attribute attr = getConstantAsAttr(constant)) {
     // These constants can be represented as attributes.
-    OpBuilder b(currentEntryBlock, currentEntryBlock->begin());
     Type type = convertType(constant->getType());
     if (auto symbolRef = attr.dyn_cast<FlatSymbolRefAttr>())
-      return bEntry.create<AddressOfOp>(UnknownLoc::get(context), type,
+      return builder.create<AddressOfOp>(UnknownLoc::get(context), type,
                                          symbolRef.getValue());
-    return bEntry.create<ConstantOp>(UnknownLoc::get(context), type, attr);
+    return builder.create<ConstantOp>(UnknownLoc::get(context), type, attr);
   }
   if (auto *cn = dyn_cast<llvm::ConstantPointerNull>(constant)) {
     Type type = convertType(cn->getType());
-    return bEntry.create<NullOp>(UnknownLoc::get(context), type);
+    return builder.create<NullOp>(UnknownLoc::get(context), type);
   }
   if (auto *gv = dyn_cast<llvm::GlobalVariable>(constant))
-    return bEntry.create<AddressOfOp>(UnknownLoc::get(context),
+    return builder.create<AddressOfOp>(UnknownLoc::get(context),
                                        processGlobal(gv));
 
-  if (auto *ce = dyn_cast<llvm::ConstantExpr>(constant)) {
-    llvm::Instruction *i = ce->getAsInstruction();
-    OpBuilder::InsertionGuard guard(builder);
-    builder.setInsertionPoint(currentEntryBlock, currentEntryBlock->begin());
+  if (auto *constExpr = dyn_cast<llvm::ConstantExpr>(constant)) {
+    llvm::Instruction *i = constExpr->getAsInstruction();
     if (failed(processInstruction(i)))
       return nullptr;
     assert(valueMapping.count(i));
 
     // If we don't remove entry of `i` here, it's totally possible that the
     // next time llvm::ConstantExpr::getAsInstruction is called again, which
     // always allocates a new Instruction, memory address of the newly
     // created Instruction might be the same as `i`. Making processInstruction
     // falsely believe that the new Instruction has been processed before
     // and raised an assertion error.
     Value value = valueMapping[i];
     valueMapping.erase(i);
     // Remove this zombie LLVM instruction now, leaving us only with the MLIR
     // op.
     i->deleteValue();
     return value;
   }
   if (auto *ue = dyn_cast<llvm::UndefValue>(constant)) {
     Type type = convertType(ue->getType());
-    return bEntry.create<UndefOp>(UnknownLoc::get(context), type);
+    return builder.create<UndefOp>(UnknownLoc::get(context), type);
   }
 
   if (isa<llvm::ConstantAggregate>(constant) ||
       isa<llvm::ConstantAggregateZero>(constant)) {
     unsigned numElements = constant->getNumOperands();
     std::function<llvm::Constant *(unsigned)> getElement =
         [&](unsigned index) -> llvm::Constant * {
       return constant->getAggregateElement(index);
if (auto *caz = dyn_cast<llvm::ConstantAggregateZero>(constant)) {
       };
     }
 
     // Generate a llvm.undef as the root value first.
     Type rootType = convertType(constant->getType());
     bool useInsertValue = rootType.isa<LLVMArrayType, LLVMStructType>();
     assert((useInsertValue || LLVM::isCompatibleVectorType(rootType)) &&
            "unrecognized aggregate type");
-    Value root = bEntry.create<UndefOp>(UnknownLoc::get(context), rootType);
+    Value root = builder.create<UndefOp>(UnknownLoc::get(context), rootType);
     for (unsigned i = 0; i < numElements; ++i) {
       llvm::Constant *element = getElement(i);
       Value elementValue = processConstant(element);
       if (!elementValue)
         return nullptr;
       if (useInsertValue) {
-        root = bEntry.create<InsertValueOp>(UnknownLoc::get(context), root,
+        root = builder.create<InsertValueOp>(UnknownLoc::get(context), root,
                                              elementValue, i);
       } else {
-        Attribute indexAttr = bEntry.getI32IntegerAttr(static_cast<int32_t>(i));
-        Value indexValue = bEntry.create<ConstantOp>(
-            UnknownLoc::get(context), bEntry.getI32Type(), indexAttr);
+        Attribute indexAttr =
+            builder.getI32IntegerAttr(static_cast<int32_t>(i));
+        Value indexValue = builder.create<ConstantOp>(
+            UnknownLoc::get(context), builder.getI32Type(), indexAttr);
         if (!indexValue)
           return nullptr;
-        root = bEntry.create<InsertElementOp>(
+        root = builder.create<InsertElementOp>(
             UnknownLoc::get(context), rootType, root, elementValue, indexValue);
       }
     }
     return root;
   }
 
   emitError(UnknownLoc::get(context))
       << "unhandled constant: " << diag(*constant);
LogicalResult Importer::processFunction(llvm::Function *func) {
   auto functionType =
       convertType(func->getFunctionType()).dyn_cast<LLVMFunctionType>();
   if (func->isIntrinsic() && isConvertibleIntrinsic(func->getIntrinsicID()))
     return success();
 
   bool dsoLocal = func->hasLocalLinkage();
   CConv cconv = convertCConvFromLLVM(func->getCallingConv());
 
-  builder.setInsertionPoint(module.getBody(), getFuncInsertPt());
+  // Insert the function at the end of the module.
+  OpBuilder::InsertionGuard guard(builder);
+  builder.setInsertionPoint(module.getBody(), getFunctionInsertionPoint());
+
   LLVMFuncOp funcOp = builder.create<LLVMFuncOp>(
       UnknownLoc::get(context), func->getName(), functionType,
       convertLinkageFromLLVM(func->getLinkage()), dsoLocal, cconv);
 
   for (const auto &it : llvm::enumerate(functionType.getParams())) {
     llvm::SmallVector<NamedAttribute, 1> argAttrs;
     if (auto *type = func->getParamByValType(it.index())) {
       Type mlirType = convertType(type);
if (func->isDeclaration())
     return success();
 
   // Eagerly create all blocks.
   for (llvm::BasicBlock &bb : *func) {
     Block *block =
         builder.createBlock(&funcOp.getBody(), funcOp.getBody().end());
     mapBlock(&bb, block);
   }
-  currentEntryBlock = &funcOp.getFunctionBody().getBlocks().front();
 
   // Add function arguments to the entry block.
   for (const auto &it : llvm::enumerate(func->args())) {
     BlockArgument blockArg = funcOp.getFunctionBody().addArgument(
         functionType.getParamType(it.index()), funcOp.getLoc());
     mapValue(&it.value(), blockArg);
   }
 

mlir/test/Target/LLVMIR/Import/intrinsic.ll

 ; RUN: mlir-translate -import-llvm %s | FileCheck %s
 
-define void @intrinsics() {
-  ret void
-}
-
 ; CHECK-LABEL:  llvm.func @fmuladd_test
 define void @fmuladd_test(float %0, float %1, <8 x float> %2, i8* %3) {
   ; CHECK: "llvm.intr.fmuladd"(%{{.*}}, %{{.*}}, %{{.*}}) : (f32, f32, f32) -> f32
   %5 = call float @llvm.fmuladd.f32(float %0, float %1, float %0)
   ; CHECK: "llvm.intr.fmuladd"(%{{.*}}, %{{.*}}, %{{.*}}) : (vector<8xf32>, vector<8xf32>, vector<8xf32>) -> vector<8xf32>
   %6 = call <8 x float> @llvm.fmuladd.v8f32(<8 x float> %2, <8 x float> %2, <8 x float> %2)
   ; CHECK: "llvm.intr.fma"(%{{.*}}, %{{.*}}, %{{.*}}) : (f32, f32, f32) -> f32
   %7 = call float @llvm.fma.f32(float %0, float %1, float %0)