[mlir][llvm] Call the instruction builders from a static method (NFC).

[gysit]

Summary

Extract a static method to call the MLIR builders that translate LLVM IR
instructions to MLIR LLVM dialect operations. This change ensures the
MLIR builders have to use the moduleImport argument rather than calling
the import methods directly. As a result, both the intrinsic and the
instruction MLIR builders have to use a moduleImport argument and none
of them has direct access to private moduleImport methods. The revision
thus enforces consistent MLIR builder implementations for instructions
and intrinsics.

The revision also moves parseDataLayoutAlignment closer to its use.

[Dinistro]

Ensuring that the mlirBuilders cannot access private members and thus forcing them to use the more stable public interface of ModuleImport is a good idea. LGTM!

mlir/lib/Target/LLVMIR/ModuleImport.cpp

235

Ultra NIT: There seem to be two spaces after the comma

mlir/include/mlir/Target/LLVMIR/ModuleImport.h

private:
   /// Imports the magic globals "global_ctors" and "global_dtors".
   LogicalResult convertGlobalCtorsAndDtors(llvm::GlobalVariable *globalVar);
   /// Returns personality of `func` as a FlatSymbolRefAttr.
   FlatSymbolRefAttr getPersonalityAsAttr(llvm::Function *func);
   /// Imports `bb` into `block`, which must be initially empty.
   LogicalResult processBasicBlock(llvm::BasicBlock *bb, Block *block);
   /// Converts an LLVM intrinsic to an MLIR LLVM dialect operation if an MLIR
   /// counterpart exists. Otherwise, returns failure.
-  LogicalResult convertIntrinsic(OpBuilder &odsBuilder, llvm::CallInst *inst);
+  LogicalResult convertIntrinsic(llvm::CallInst *inst);
   /// Converts an LLVM instruction to an MLIR LLVM dialect operation if an MLIR
   /// counterpart exists. Otherwise, returns failure.
-  LogicalResult convertInstruction(OpBuilder &odsBuilder,
-                                   llvm::Instruction *inst);
+  LogicalResult convertInstruction(llvm::Instruction *inst);
   /// Converts the metadata attached to the original instruction `inst` if
   /// a dialect interfaces supports the specific kind of metadata and attaches
   /// the resulting dialect attributes to the converted operation `op`. Emits a
   /// warning if the conversion of a supported metadata kind fails.
   void setNonDebugMetadataAttrs(llvm::Instruction *inst, Operation *op);
   /// Imports `inst` and populates valueMapping[inst] with the result of the
   /// imported operation or noResultOpMapping[inst] with the imported operation
   /// if it has no result.

mlir/lib/Target/LLVMIR/ModuleImport.cpp

static constexpr StringRef getGlobalCtorsVarName() {
   return "llvm.global_ctors";
 }
 
 /// Returns the name of the global_dtors global variables.
 static constexpr StringRef getGlobalDtorsVarName() {
   return "llvm.global_dtors";
 }
 
-/// Creates an attribute containing ABI and preferred alignment numbers parsed
-/// a string. The string may be either "abi:preferred" or just "abi". In the
-/// latter case, the preferred alignment is considered equal to ABI alignment.
-static DenseIntElementsAttr parseDataLayoutAlignment(MLIRContext &ctx,
-                                                     StringRef spec) {
-  auto i32 = IntegerType::get(&ctx, 32);
-
-  StringRef abiString, preferredString;
-  std::tie(abiString, preferredString) = spec.split(':');
-  int abi, preferred;
-  if (abiString.getAsInteger(/*Radix=*/10, abi))
-    return nullptr;
-
-  if (preferredString.empty())
-    preferred = abi;
-  else if (preferredString.getAsInteger(/*Radix=*/10, preferred))
-    return nullptr;
-
-  return DenseIntElementsAttr::get(VectorType::get({2}, i32), {abi, preferred});
-}
-
 /// Returns a supported MLIR floating point type of the given bit width or null
 /// if the bit width is not supported.
 static FloatType getDLFloatType(MLIRContext &ctx, int32_t bitwidth) {
   switch (bitwidth) {
   case 16:
     return FloatType::getF16(&ctx);
   case 32:
     return FloatType::getF32(&ctx);

 
 /// Converts an array of unsigned indices to a signed integer position array.
 static SmallVector<int64_t> getPositionFromIndices(ArrayRef<unsigned> indices) {
   SmallVector<int64_t> position;
   llvm::append_range(position, indices);
   return position;
 }
 
+/// Converts the LLVM instructions that have a generated MLIR builder. Using a
+/// static implementation method called from the module import ensures the
+/// builders have to use the `moduleImport` argument and cannot directly call
+/// import methods. As a result, both the intrinsic and the instruction MLIR
+/// builders have to use the `moduleImport` argument and none of them has direct
+/// access to the private module import methods.
+static LogicalResult convertInstructionImpl(OpBuilder &odsBuilder,
+                                            llvm::Instruction *inst,
+                                            ModuleImport &moduleImport) {
+  // Copy the operands to an LLVM operands array reference for conversion.
+  SmallVector<llvm::Value *> operands(inst->operands());
+  ArrayRef<llvm::Value *> llvmOperands(operands);
+
+  // Convert all instructions that provide an MLIR builder.
+#include "mlir/Dialect/LLVMIR/LLVMOpFromLLVMIRConversions.inc"
+  return failure();
+}
+
+/// Creates an attribute containing ABI and preferred alignment numbers parsed
+/// a string. The string may be either "abi:preferred" or just "abi". In the
+/// latter case, the preferred alignment is considered equal to ABI alignment.
+static DenseIntElementsAttr parseDataLayoutAlignment(MLIRContext &ctx,
+                                                     StringRef spec) {
+  auto i32 = IntegerType::get(&ctx, 32);
+
+  StringRef abiString, preferredString;
+  std::tie(abiString, preferredString) = spec.split(':');
+  int abi, preferred;
+  if (abiString.getAsInteger(/*Radix=*/10, abi))
+    return nullptr;
+
+  if (preferredString.empty())
+    preferred = abi;
+  else if (preferredString.getAsInteger(/*Radix=*/10, preferred))
+    return nullptr;
+
+  return DenseIntElementsAttr::get(VectorType::get({2}, i32), {abi, preferred});
+}
+
 /// Translate the given LLVM data layout into an MLIR equivalent using the DLTI
 /// dialect.
 DataLayoutSpecInterface
 mlir::translateDataLayout(const llvm::DataLayout &dataLayout,
                           MLIRContext *context) {
   assert(context && "expected MLIR context");
   std::string layoutstr = dataLayout.getStringRepresentation();
 
ModuleImport::convertCallTypeAndOperands(llvm::CallBase *callInst,
   SmallVector<llvm::Value *> args(callInst->args());
   FailureOr<SmallVector<Value>> arguments = convertValues(args);
   if (failed(arguments))
     return failure();
   llvm::append_range(operands, *arguments);
   return success();
 }
 
-LogicalResult ModuleImport::convertIntrinsic(OpBuilder &odsBuilder,
-                                             llvm::CallInst *inst) {
+LogicalResult ModuleImport::convertIntrinsic(llvm::CallInst *inst) {
   if (succeeded(iface.convertIntrinsic(builder, inst, *this)))
     return success();
 
   Location loc = translateLoc(inst->getDebugLoc());
   return emitError(loc) << "unhandled intrinsic " << diag(*inst);
 }
 
-LogicalResult ModuleImport::convertInstruction(OpBuilder &odsBuilder,
-                                               llvm::Instruction *inst) {
-  // Copy the operands to an LLVM operands array reference for conversion.
-  SmallVector<llvm::Value *> operands(inst->operands());
-  ArrayRef<llvm::Value *> llvmOperands(operands);
-  ModuleImport &moduleImport = *this;
-
-  // Convert all instructions that provide an MLIR builder.
-#include "mlir/Dialect/LLVMIR/LLVMOpFromLLVMIRConversions.inc"
-
-  // Convert all remaining instructions that do not provide an MLIR builder.
+LogicalResult ModuleImport::convertInstruction(llvm::Instruction *inst) {
+  // Convert all instructions that do not provide an MLIR builder.
   Location loc = translateLoc(inst->getDebugLoc());
   if (inst->getOpcode() == llvm::Instruction::Br) {
     auto *brInst = cast<llvm::BranchInst>(inst);
 
     SmallVector<Block *> succBlocks;
     SmallVector<SmallVector<Value>> succBlockArgs;
     for (auto i : llvm::seq<unsigned>(0, brInst->getNumSuccessors())) {
       llvm::BasicBlock *succ = brInst->getSuccessor(i);
if (inst->getOpcode() == llvm::Instruction::GetElementPtr) {
 
     Type type = convertType(inst->getType());
     auto gepOp = builder.create<GEPOp>(loc, type, sourceElementType, *basePtr,
                                        indices, gepInst->isInBounds());
     mapValue(inst, gepOp);
     return success();
   }
 
+  // Convert all instructions that have an mlirBuilder.
+  if (succeeded(convertInstructionImpl(builder, inst, *this)))
+    return success();
+
   return emitError(loc) << "unhandled instruction " << diag(*inst);
 }
 
 LogicalResult ModuleImport::processInstruction(llvm::Instruction *inst) {
   // FIXME: Support uses of SubtargetData.
   // FIXME: Add support for call / operand attributes.
   // FIXME: Add support for the indirectbr, cleanupret, catchret, catchswitch,
   // callbr, vaarg, landingpad, catchpad, cleanuppad instructions.
 
   // Convert LLVM intrinsics calls to MLIR intrinsics.
   if (auto *callInst = dyn_cast<llvm::CallInst>(inst)) {
     llvm::Function *callee = callInst->getCalledFunction();
     if (callee && callee->isIntrinsic())
-      return convertIntrinsic(builder, callInst);
+      return convertIntrinsic(callInst);
   }
 
   // Convert all remaining LLVM instructions to MLIR operations.
-  return convertInstruction(builder, inst);
+  return convertInstruction(inst);
 }
 
 FlatSymbolRefAttr ModuleImport::getPersonalityAsAttr(llvm::Function *f) {
   if (!f->hasPersonalityFn())
     return nullptr;
 
   llvm::Constant *pf = f->getPersonalityFn();