Apply last round of reviews

Author: bcardosolopes

mlir/include/mlir/Target/LLVMIR/LLVMImportInterface.h

@@ -98,12 +98,6 @@ class LLVMImportDialectInterface
 /// efficient dispatch.
 class LLVMImportInterface
     : public DialectInterfaceCollection<LLVMImportDialectInterface> {
-private:
-  /// Generate llvm.call_intrinsic when no supporting dialect available.
-  LogicalResult
-  convertUnregisteredIntrinsic(OpBuilder &builder, llvm::CallInst *inst,
-                               LLVM::ModuleImport &moduleImport) const;
-
 public:
   using Base::Base;
 
@@ -239,6 +233,11 @@ class LLVMImportInterface
   }
 
 private:
+  /// Generate llvm.call_intrinsic when no supporting dialect available.
+  static LogicalResult
+  convertUnregisteredIntrinsic(OpBuilder &builder, llvm::CallInst *inst,
+                               LLVM::ModuleImport &moduleImport);
+
   DenseMap<unsigned, Dialect *> intrinsicToDialect;
   DenseMap<unsigned, const LLVMImportDialectInterface *> instructionToDialect;
   DenseMap<unsigned, SmallVector<Dialect *, 1>> metadataToDialect;

mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMIRToLLVMTranslation.cpp

@@ -80,48 +80,6 @@ static LogicalResult convertIntrinsicImpl(OpBuilder &odsBuilder,
   return failure();
 }
 
-/// Converts the LLVM intrinsic to a generic LLVM intrinsic call using
-/// llvm.intrinsic_call. Returns failure otherwise.
-static LogicalResult
-convertUnregisteredIntrinsicImpl(OpBuilder &odsBuilder, llvm::CallInst *inst,
-                                 LLVM::ModuleImport &moduleImport) {
-  StringRef intrinName = inst->getCalledFunction()->getName();
-
-  SmallVector<llvm::Value *> args(inst->args());
-  ArrayRef<llvm::Value *> llvmOperands(args);
-
-  SmallVector<llvm::OperandBundleUse> llvmOpBundles;
-  llvmOpBundles.reserve(inst->getNumOperandBundles());
-  for (unsigned i = 0; i < inst->getNumOperandBundles(); ++i)
-    llvmOpBundles.push_back(inst->getOperandBundleAt(i));
-
-  SmallVector<Value> mlirOperands;
-  SmallVector<NamedAttribute> mlirAttrs;
-  if (failed(moduleImport.convertIntrinsicArguments(
-          llvmOperands, llvmOpBundles, false, {}, {}, mlirOperands, mlirAttrs)))
-    return failure();
-
-  Type results = moduleImport.convertType(inst->getType());
-  auto op = odsBuilder.create<::mlir::LLVM::CallIntrinsicOp>(
-      moduleImport.translateLoc(inst->getDebugLoc()), results,
-      StringAttr::get(odsBuilder.getContext(), intrinName),
-      ValueRange{mlirOperands}, FastmathFlagsAttr{});
-
-  moduleImport.setFastmathFlagsAttr(inst, op);
-
-  // Update importer tracking of results.
-  unsigned numRes = op.getNumResults();
-  if (numRes == 1)
-    moduleImport.mapValue(inst) = op.getResult(0);
-  else if (numRes == 0)
-    moduleImport.mapNoResultOp(inst);
-  else
-    return op.emitError(
-        "expected at most one result from target intrinsic call");
-
-  return success();
-}
-
 /// Returns the list of LLVM IR metadata kinds that are convertible to MLIR LLVM
 /// dialect attributes.
 static ArrayRef<unsigned> getSupportedMetadataImpl(llvm::LLVMContext &context) {
@@ -411,8 +369,42 @@ static LogicalResult setIntelReqdSubGroupSizeAttr(Builder &builder,
 
 LogicalResult mlir::LLVMImportInterface::convertUnregisteredIntrinsic(
     OpBuilder &builder, llvm::CallInst *inst,
-    LLVM::ModuleImport &moduleImport) const {
-  return convertUnregisteredIntrinsicImpl(builder, inst, moduleImport);
+    LLVM::ModuleImport &moduleImport) {
+  StringRef intrinName = inst->getCalledFunction()->getName();
+
+  SmallVector<llvm::Value *> args(inst->args());
+  ArrayRef<llvm::Value *> llvmOperands(args);
+
+  SmallVector<llvm::OperandBundleUse> llvmOpBundles;
+  llvmOpBundles.reserve(inst->getNumOperandBundles());
+  for (unsigned i = 0; i < inst->getNumOperandBundles(); ++i)
+    llvmOpBundles.push_back(inst->getOperandBundleAt(i));
+
+  SmallVector<Value> mlirOperands;
+  SmallVector<NamedAttribute> mlirAttrs;
+  if (failed(moduleImport.convertIntrinsicArguments(
+          llvmOperands, llvmOpBundles, false, {}, {}, mlirOperands, mlirAttrs)))
+    return failure();
+
+  Type results = moduleImport.convertType(inst->getType());
+  auto op = builder.create<::mlir::LLVM::CallIntrinsicOp>(
+      moduleImport.translateLoc(inst->getDebugLoc()), results,
+      StringAttr::get(builder.getContext(), intrinName),
+      ValueRange{mlirOperands}, FastmathFlagsAttr{});
+
+  moduleImport.setFastmathFlagsAttr(inst, op);
+
+  // Update importer tracking of results.
+  unsigned numRes = op.getNumResults();
+  if (numRes == 1)
+    moduleImport.mapValue(inst) = op.getResult(0);
+  else if (numRes == 0)
+    moduleImport.mapNoResultOp(inst);
+  else
+    return op.emitError(
+        "expected at most one result from target intrinsic call");
+
+  return success();
 }
 namespace {
 

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

@@ -13,7 +13,7 @@ define dso_local void @t0(ptr %a) {
 ; CHECK:   llvm.return
 ; CHECK: }
 
-; -----
+; // -----
 
 declare <8 x i8> @llvm.aarch64.neon.uabd.v8i8(<8 x i8>, <8 x i8>)
 
@@ -27,7 +27,7 @@ define dso_local <8 x i8> @t1(<8 x i8> %lhs, <8 x i8> %rhs) {
 ; CHECK:   llvm.return %[[R]] : vector<8xi8>
 ; CHECK: }
 
-; -----
+; // -----
 
 declare void @llvm.aarch64.neon.st2.v8i8.p0(<8 x i8>, <8 x i8>, ptr)
 
@@ -41,7 +41,7 @@ define dso_local void @t2(<8 x i8> %lhs, <8 x i8> %rhs, ptr %a) {
 ; CHECK:   llvm.return
 ; CHECK: }
 
-; -----
+; // -----
 
 declare void @llvm.gcroot(ptr %arg1, ptr %arg2)
 define void @gctest() gc "example" {
@@ -53,7 +53,7 @@ define void @gctest() gc "example" {
 ; CHECK-LABEL: @gctest
 ; CHECK: llvm.call_intrinsic "llvm.gcroot"({{.*}}, {{.*}}) : (!llvm.ptr, !llvm.ptr) -> !llvm.void
 
-; -----
+; // -----
 
 ; Test we get the supported version, not the unregistered one.