[mlir] Add call_intrinsic op to LLVMIIR

[electriclilies]

Summary

The call_intrinsic op allows us to call LLVM intrinsics from the LLVMDialect without implementing a new op every time.

[Mogball]

nice

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

683–685

This is the new style for headers (LLVM dialect is very old!)

686

can you name this LLVM_CallIntrinsicOp? It affects the name of the C++ class

686

can you add a summary and description field to document the op?

692–694

701

can you outline this to a C++ file? I think you can add a function to mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp where the generated code gets pasted

[electriclilies]

Moved some logic from the builder into a function; responding to comments

[electriclilies]

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

701

I moved a bunch of it to getOverloadedDeclaration, let me know what you think

[Mogball]

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

697

An intrinsic that doesn't exist shouldn't cause the exporter to assert. Can you have it fail gracefully with an error?

703

please drop trivial braces

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

271 ↗

(On Diff #472404)

please drop trivial braces

mlir/test/Dialect/LLVMIR/call-intrin.mlir

15

What happens when you try to export an intrinsic that doesn't exist on all systems? I'm worried this test might fail on some systems

[lattner]

Very exciting Lily, thank you for implementing this!

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

266 ↗

(On Diff #472404)

Plz only eval getNumOperands once per iteration and prefer preincrement:

for (size_t i = 0, e = opInst.getNumOperands(); i != e; ++i)

It is a micro thing but helps with consistency.
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
https://llvm.org/docs/CodingStandards.html#prefer-preincrement

283 ↗

(On Diff #472404)

Do you need the llvm:: namespace qualifiers in the .cpp file? I think they can all be dropped

283 ↗

(On Diff #472404)

Can you pass Table directly into matchIntrinsicSignature? I think they implicitly convert and you can get rid of TableRef

mlir/test/Dialect/LLVMIR/call-intrin.mlir

15

I think it will always work but am not sure, how do we test this? Maybe add a weird target intrinsic that you don't have configured to check?

[electriclilies]

Fix error messages + comments

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

283 ↗

(On Diff #472404)

I do need the llvm:: namespace qualifiers unfortunately; a lot of other functions in this file use them too. :(

And I'm not sure why, but if I try to pass Table in directly, it doesn't compile with this error--

/home/lily/llvm-project/mlir/lib/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.cpp:283:52: error: non-const lvalue reference to type 'ArrayRef<llvm::Intrinsic::IITDescriptor>' cannot bind to a value of unrelated type 'SmallVector<llvm::Intrinsic::IITDescriptor, 8>'
  if (llvm::Intrinsic::matchIntrinsicSignature(FT, Table,

There are other functions that called matchIntrinsicSignature like this as well

mlir/test/Dialect/LLVMIR/call-intrin.mlir

15

I'm not 100% sure what the behavior is-- if the intrinsic isn't on a particular machine then then it might just return 0 when it tries to get the intrinsic. I can change this to a different test

15

Earlier today I was trying to use some intrinsics that were listed in llvmint, and getting back 0 for the id of those intrinsics. Turns out they were ones that weren't in ""LLVMConvertibleLLVMIRIntrinsics.inc".

I'm not sure if "LLVMConvertibleLLVMIRIntrinsics.inc" is dependent on the target or if it's just a static list that's the same across all backends.

[electriclilies]

Make errors a bit better

[electriclilies]

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

697

\

705

If this does fail, then there will probably be an error in > builder.CreateCall <. I'm not sure how to return early from a builder..

[electriclilies]

Make error reporting work properly

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

700

Currently this else is requ

705

Nevermind, I got this to work properly.

[lattner]

I do need the llvm:: namespace qualifiers unfortunately; a lot of other functions in this file use them too. :(

Ok!

[electriclilies]

clang-format

[rriddle]

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

689

To provide a similar summary to the normal call op.

690–692

698–699

700–701

How does success translate to error propagating correctly?

701

I would move all of this out of line if possible.

706–708

718

Drop the newline here.

725–727

I would actually advocate for not having this on the op definition, because it requires having LLVM IR linked and available (which I don't think we want really in the LLVM dialect, even if it currently does now).

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

261–262 ↗

(On Diff #472717)

Can you document this method? The contract seems specific, also can you have a more specific operation type for opInst?

286 ↗

(On Diff #472717)

Do you have tests for these error cases?

286–287 ↗

(On Diff #472717)

290–293 ↗

(On Diff #472717)

[electriclilies]

Comments + add more tests

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

700–701

It actually should be failure(), my error checking was not quite right and it was working for failure() and not success()

725–727

OK, I can inline it

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

261–262 ↗

(On Diff #472717)

It looks like opInst is the exact same thing as op, so I can actually just get rid of opInst.

286 ↗

(On Diff #472717)

I added a test for intrinsic type is not a match.

For the variadic-ness one, I can't find a variadic intrinsic, so I didn't add a test. It seems like variadic intrinsics are uncommon, though, and I'm not sure how to support them. I updated the error message to say that we don't support calling variadic intrinsics.

[electriclilies]

Move most of builder out of .td file

[electriclilies]

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

701

OK, I moved it all into another C++ function.

706–708

I removed the comment so I think I don't need the {} anymore

[electriclilies]

format

[rriddle]

As something that better enables scaling to the many intrinsics, this LGTM conceptually.

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

687–688

Could you sink these below the summary+description? Those fields should generally always be at the top.

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

269–271 ↗

(On Diff #472768)

Can you do something like:

280 ↗

(On Diff #472768)

Please use camelCase for variable names. Same below.

284 ↗

(On Diff #472768)

Do you need this temporary variable? Can you just pass Table inline?

312–313 ↗

(On Diff #472768)

286 ↗

(On Diff #472717)

You could try llvm.localescape which is variadic IIRC. There are a few others as well.

286–287 ↗

(On Diff #472717)

The return op.emitOpError still looks unresolved?

[electriclilies]

comments

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

284 ↗

(On Diff #472768)

Yup, it is necessary unfortunately, it won't convert implicilty

286–287 ↗

(On Diff #472717)

whoops, misread the suggestion. should be resolved now

[electriclilies]

add variadic test + remove variadic check since it's unneeded

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

286 ↗

(On Diff #472717)

Thanks! I added a test for localescope, and it seems like things are working fine for variadic localescope, so I'm just going to remove the check. I don't think it's possible to trigger it.

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td

uint64_t getSrcVectorBitWidth() {
       return getVectorBitWidth(getSrcvec().getType());
     }
     uint64_t getResVectorBitWidth() {
       return getVectorBitWidth(getRes().getType());
     }
   }];
 }
 
 //
+// Call an LLVM intrinsic
+//
+def LLVM_call_intrinsic : LLVM_Op<"call_intrinsic", [Pure]> {
+  let arguments = (ins StrAttr:$intrin, Variadic<LLVM_Type>:$args);
+  let results = (outs Variadic<LLVM_Type>:$results);
+  let llvmBuilder = [{
+    llvm::Module *module = builder.GetInsertBlock()->getModule();
+    llvm::Intrinsic::ID id = op.getIntrinsicEnum();
+    if (!id) {
+      assert(false && "Couldn't find the intrinsic!\n");
+    }
+    llvm::Function *fn;
+    if (!llvm::Intrinsic::isOverloaded(id)) {
+        fn = llvm::Intrinsic::getDeclaration(module, id, {});
+    } else {
+      SmallVector<llvm::Intrinsic::IITDescriptor, 8> Table;
+      getIntrinsicInfoTableEntries(id, Table);
+      ArrayRef<llvm::Intrinsic::IITDescriptor> TableRef = Table;
+
+      SmallVector<llvm::Type *, 8> opTys;
+      for (size_t i = 0; i < opInst.getNumOperands(); i++) {
+        opTys.push_back(moduleTranslation.convertType(
+          opInst.getOperand(i).getType()));
+      }
+
+      llvm::Type* resTy;
+      if (opInst.getNumResults() == 0) {
+        resTy = llvm::Type::getVoidTy(module->getContext());
+      } else {
+        resTy = moduleTranslation.convertType(opInst.getResult(0).getType());
+      }
+      llvm::FunctionType* FT = llvm::FunctionType::get(resTy, opTys, false);
+
+      SmallVector<llvm::Type *, 8> argTys;
+      if (llvm::Intrinsic::matchIntrinsicSignature(FT, TableRef, argTys) !=
+       llvm::Intrinsic::MatchIntrinsicTypesResult::MatchIntrinsicTypes_Match) {
+         assert (false && "Intrinsic type is not a match!\n");
+      }
+      if (llvm::Intrinsic::matchIntrinsicVarArg(FT->isVarArg(), TableRef)) {
+        assert (false && "Intrinsic variadic-ness is not a match!\n");
+      }
+
+      ArrayRef<llvm::Type *> ArgTys = argTys;
+      fn = llvm::Intrinsic::getDeclaration(module, id, ArgTys);
+    }
+
+    auto *inst = builder.CreateCall(fn,
+                  moduleTranslation.lookupValues(opInst.getOperands()));
+    if (opInst.getNumResults() == 1) {
+        moduleTranslation.mapValue(op->getResults().front())  = inst;
+    }
+  }];
+  let assemblyFormat = [{
+    $intrin `(` $args `)` `:` functional-type($args, $results) attr-dict
+  }];
+
+  let extraClassDeclaration = [{
+    llvm::Intrinsic::ID getIntrinsicEnum() {
+      return llvm::Function::lookupIntrinsicID(getIntrinAttr());
+    }
+  }];
+}
+
+//
 // LLVM Vector Predication operations.
 //
 
 class LLVM_VPBinaryBase<string mnem, Type element>
     : LLVM_OneResultIntrOp<"vp." # mnem, [0], [], [Pure]>,
       Arguments<(ins LLVM_VectorOf<element>:$lhs, LLVM_VectorOf<element>:$rhs,
                      LLVM_VectorOf<I1>:$mask, I32:$evl)>;
 

mlir/test/Dialect/LLVMIR/call-intrin.mlir

+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+
+// CHECK: ; ModuleID = 'LLVMDialectModule'
+// CHECK: source_filename = "LLVMDialectModule"
+// CHECK: declare ptr @malloc(i64)
+// CHECK: declare void @free(ptr)
+// CHECK: define <4 x float> @round_sse41() {
+// CHECK:  %1 = call <4 x float> @llvm.x86.sse41.round.ss(<4 x float> <float 0x3FC99999A0000000, float 0x3FC99999A0000000, float 0x3FC99999A0000000, float 0x3FC99999A0000000>, <4 x float> <float 0x3FC99999A0000000, float 0x3FC99999A0000000, float 0x3FC99999A0000000, float 0x3FC99999A0000000>, i32 1)
+// CHECK:  ret <4 x float> %1
+// CHECK: }
+llvm.func @round_sse41() -> vector<4xf32> {
+    %0 = llvm.mlir.constant(1 : i32) : i32
+    %1 = llvm.mlir.constant(dense<0.2> : vector<4xf32>) : vector<4xf32>
+    %res = llvm.call_intrinsic "llvm.x86.sse41.round.ss"(%1, %1, %0) : (vector<4xf32>, vector<4xf32>, i32) -> vector<4xf32> {}
+    llvm.return %res: vector<4xf32>
+}
+
+// -----
+
+// CHECK: ; ModuleID = 'LLVMDialectModule'
+// CHECK: source_filename = "LLVMDialectModule"
+
+// CHECK: declare ptr @malloc(i64)
+
+// CHECK: declare void @free(ptr)
+
+// CHECK: define float @round_overloaded() {
+// CHECK:   %1 = call float @llvm.round.f32(float 1.000000e+00)
+// CHECK:   ret float %1
+// CHECK: }
+llvm.func @round_overloaded() -> f32 {
+    %0 = llvm.mlir.constant(1.0 : f32) : f32
+    %res = llvm.call_intrinsic "llvm.round"(%0) : (f32) -> f32 {}
+    llvm.return %res: f32
+}
+
+// -----
+
+// CHECK: ; ModuleID = 'LLVMDialectModule'
+// CHECK: source_filename = "LLVMDialectModule"
+// CHECK: declare ptr @malloc(i64)
+// CHECK: declare void @free(ptr)
+// CHECK: define void @lifetime_start() {
+// CHECK:   %1 = alloca float, i8 1, align 4
+// CHECK:   call void @llvm.lifetime.start.p0(i64 4, ptr %1)
+// CHECK:   ret void
+// CHECK: }
+llvm.func @lifetime_start() {
+    %0 = llvm.mlir.constant(4 : i64) : i64
+    %1 = llvm.mlir.constant(1 : i8) : i8
+    %2 = llvm.alloca %1 x f32 : (i8) -> !llvm.ptr
+    llvm.call_intrinsic "llvm.lifetime.start"(%0, %2) : (i64, !llvm.ptr) -> () {}
+    llvm.return
+}