[mlir][LLVMIR] Add support for InlineAsmOp

[nicolasvasilache]

Summary

The InlineAsmOp mirrors the underlying LLVM semantics with a notable
exception: the embedded asm_string is not allowed to define or reference
any symbol or any global variable: only the operands of the op may be read,
written, or referenced.
Attempting to define or reference any symbol or any global behavior is
considered undefined behavior at this time.

The asm dialect syntax is currently specified with an integer (0 [default] for the "att dialect", 1 for the intel dialect) to circumvent the ODS limitation on string enums.

Translation to LLVM is provided and raises the fact that the asm constraints string must be well-formed with respect to in/out operands. No check is performed on the asm_string.

An InlineAsm instruction in LLVM is a special call operation to a function that is constructed on the fly.
It does not fit the current model of MLIR calls with symbols.
As a consequence, the current implementation constructs the function type in ModuleTranslation.cpp.
This should be refactored in the future.

The mlir-cpu-runner is augmented with the global initialization of the X86 asm parser to allow proper execution in JIT mode. Previously, only the X86 asm printer was initialized.

[rriddle]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

1221

Please use a multi-line string instead:

[{ ... }]

[mehdi_amini]

mlir/test/Dialect/Neon/roundtrip.mlir

25 ↗

(On Diff #307811)

Can you clarify how this test relates to the inline ASM?

[nicolasvasilache]

Drop spurious test.
Connect asm parser to make integration test run properly.

[nicolasvasilache]

mlir/test/Dialect/Neon/roundtrip.mlir

25 ↗

(On Diff #307811)

Exactly as you'd expect, it doesn't.
Thanks ! :)

[ftynse]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

1209

Just LLVM_Op, the zero result option makes everything assume that there are no results.

1219

What's the intended behavior with results? ODS allows variadic here (none of the other LLVM dialect ops allow for multiple results AFAIR) and there is packing in _type conversion_ but there is no support for extracting the results of a struct, and there are no tests.

I would suggest having Optional<LLVM_Type> instead and always return a struct if there are multiple results. Then explicitly extract values from the struct if necessary. This will be more consistent with the rest of the LLVM dialect, and simplify the function-type creation code a bit.

mlir/integration_test/Dialect/LLVMIR/CPU/test-inline-asm.mlir

1

We probably need a way to disable this test if the tests are executed on something else than x86. E.g., ARM instruction is called eor and I would expect it to complain about xor.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

587

Please fix

[nicolasvasilache]

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

1219

Good point thanks!

mlir/integration_test/Dialect/LLVMIR/CPU/test-inline-asm.mlir

1

Can this be done later ?
I suspect this is premature thinking atm and it may require a longer discussion on restructuring things.
If this cannot land as is, my inclination is to remove this test for now.

[nicolasvasilache]

Address comments.

[ftynse]

mlir/integration_test/Dialect/LLVMIR/CPU/test-inline-asm.mlir

1

It should be possible to declare tests as unsupported in lit.local.cfg, we already do so for GPU tests.

If that's too much work, let's not commit it then. We seem to have a PPC64 buildbot with MLIR http://lab.llvm.org:8011/#/builders/88, and I'm pretty sure I got some mails from an ARM buildbot.

[mehdi_amini]

The InlineAsmOp mirrors the underlying LLVM semantics.

This is really scary: https://bugs.llvm.org/show_bug.cgi?id=28970

I'm a bit concerned about bringing this into MLIR without stronger constraints than what is allowed in LLVM. Unfortunately even if we document restriction, it isn't clear how we can enforce anything? The way inline assembly is an open-door to "anything" seems quite scary to me.

[mehdi_amini]

mlir/integration_test/Dialect/LLVMIR/CPU/test-inline-asm.mlir

1

Please create a X86 subdirectory and add the right configuration restriction in lit. We already have this for other tests.

[nicolasvasilache]

Address comments.

[nicolasvasilache]

I'm a bit concerned about bringing this into MLIR without stronger constraints than what is allowed in LLVM. Unfortunately even if we document restriction, it isn't clear how we can enforce anything? The way inline assembly is an open-door to "anything" seems quite scary to me.

Ack, certain things are prone to footgunning: asm (inline or not), pointers, bitcast etc.
In the meantime, production code on virtually all HW relies some inline asm.
The fact that https://bugs.llvm.org/show_bug.cgi?id=28970 has not moved for 4+years does not make me confident we should wait for something additional to happen on this front.

mlir/integration_test/Dialect/LLVMIR/CPU/test-inline-asm.mlir

1

I fail to see how this can be done under integration_tests which has the structure integration_tests/xxx/CPU: the whole integration_tests dir is activated by -DMLIR_INCLUDE_X86_INTEGRATION_TESTS.

I don't want to be in the business of reorganizing those test in this revision and

1

scratch that, I made it work but forgot to drop this message.

[rovka]

Hi,

This seems to break several buildbots that don't build the x86 backend, e.g. http://lab.llvm.org:8011/#/builders/135/builds/90.
Could you please take a look?

Thanks!

[nicolasvasilache]

In D92166#2422886, @rovka wrote:

Hi,

This seems to break several buildbots that don't build the x86 backend, e.g. http://lab.llvm.org:8011/#/builders/135/builds/90.
Could you please take a look?

Thanks!

Hi @rovka sorry about that, I just pushed 78c71187465a8e877d2e07d462b45a19363fb782.
Is there a link I can follow to see whether this is enough ?

[nicolasvasilache]

Poking a bit around I see this build: http://lab.llvm.org:8011/#/builders/135/builds/110

The issue your reported now seems resolved on your end.

[mehdi_amini]

Please don't land patches while there are discussions in flight.

In D92166#2422116, @nicolasvasilache wrote:

I'm a bit concerned about bringing this into MLIR without stronger constraints than what is allowed in LLVM. Unfortunately even if we document restriction, it isn't clear how we can enforce anything? The way inline assembly is an open-door to "anything" seems quite scary to me.

Ack, certain things are prone to footgunning: asm (inline or not), pointers, bitcast etc.
In the meantime, production code on virtually all HW relies some inline asm.

FYI: I see the analogy you're making here as a gross simplification and mixing of things that have nothing to do with each other.

The fact that https://bugs.llvm.org/show_bug.cgi?id=28970 has not moved for 4+years does not make me confident we should wait for something additional to happen on this front.

I strongly disagree here: MLIR isn't LLVM and we don't have to support everything that GCC has supported so far.

Also, your patch yields

tools/mlir/include/mlir/Dialect/LLVMIR/LLVMConversionEnumsFromLLVM.inc:1:33: warning: unused function 'convertAsmDialectFromLLVM' [-Wunused-function]
static ::mlir::LLVM::AsmDialect convertAsmDialectFromLLVM(::llvm::InlineAsm::AsmDialect value) {
                                ^

This seems like you're missing some implementation (and testing coverage).

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td

let llvmBuilder = [{
     llvm::LLVMContext &llvmContext = builder.getContext();
     builder.CreateFence(getLLVMAtomicOrdering($ordering),
       llvmContext.getOrInsertSyncScopeID($syncscope));
   }];
   let parser = [{ return parseFenceOp(parser, result); }];
   let printer = [{ printFenceOp(p, *this); }];
   let verifier = "return ::verify(*this);";
 }
+
+def AsmATT : LLVM_EnumAttrCase<
+  /*string cppSym=*/"AD_ATT", /*string irSym=*/"att",
+  /*string llvmSym=*/"AD_ATT", /*int val=*/0>;
+def AsmIntel : LLVM_EnumAttrCase<
+  /*string cppSym=*/"AD_Intel", /*string irSym=*/"intel",
+  /*string llvmSym=*/"AD_Intel", /*int val=*/1>;
+def AsmATTOrIntel : LLVM_EnumAttr<
+  /*string name=*/"AsmDialect",
+  /*string llvmName=*/"::llvm::InlineAsm::AsmDialect",
+  /*string description=*/"ATT (0) or Intel (1) asm dialect",
+  /*list<LLVM_EnumAttrCase> cases=*/[AsmATT, AsmIntel]> {
+  let cppNamespace = "::mlir::LLVM";
+}
+
+def LLVM_InlineAsmOp : LLVM_ZeroResultOp<"inline_asm", []> {
+  let arguments = (
+    ins Variadic<LLVM_Type>:$operands,
+        StrAttr:$asm_string,
+        StrAttr:$constraints,
+        UnitAttr:$has_side_effects,
+        UnitAttr:$is_align_stack,
+        OptionalAttr<
+          DefaultValuedAttr<AsmATTOrIntel, "AsmDialect::AD_ATT">>:$asm_dialect);
+
+  let results = (outs Variadic<LLVM_Type>:$res);
+
+  let assemblyFormat = [{
+    (`has_side_effects` $has_side_effects^)?
+    (`is_align_stack` $is_align_stack^)?
+    (`asm_dialect` `=` $asm_dialect^)?
+    attr-dict
+    $asm_string `,` $constraints
+    operands `:` functional-type(operands, results)
+   }];
+}
 #endif // LLVMIR_OPS

mlir/integration_test/Dialect/LLVMIR/CPU/test-inline-asm.mlir

+// RUN: mlir-cpu-runner %s -e entry -entry-point-result=void  \
+// RUN: -shared-libs=%mlir_integration_test_dir/libmlir_c_runner_utils%shlibext | \
+// RUN: FileCheck %s
+
+module {
+  llvm.func @printI64(!llvm.i64)
+  llvm.func @entry()  {
+    %c2 = llvm.mlir.constant(-42: i64) :!llvm.i64
+    %val = llvm.inline_asm "xor $0, $0", "=r,r" %c2 :
+      (!llvm.i64) -> !llvm.i64
+
+    // CHECK: 0
+    llvm.call @printI64(%val) : (!llvm.i64) -> ()
+    llvm.return
+  }
+}

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp

 #include "mlir/IR/StandardTypes.h"
 #include "mlir/Target/LLVMIR.h"
 #include "mlir/Target/LLVMIR/TypeTranslation.h"
 #include "mlir/Translation.h"
 
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
+#include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/SourceMgr.h"
 
 using namespace mlir;
 using namespace mlir::LLVM;

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

 #include "llvm/ADT/PostOrderIterator.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/IRBuilder.h"
+#include "llvm/IR/InlineAsm.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 
 using namespace mlir;
 using namespace mlir::LLVM;
if (isa<LLVM::CallOp>(opInst)) {
     if (opInst.getNumResults() != 0) {
       valueMapping[opInst.getResult(0)] = result;
       return success();
     }
     // Check that LLVM call returns void for 0-result functions.
     return success(result->getType()->isVoidTy());
   }
 
+  if (auto inlineAsmOp = dyn_cast<LLVM::InlineAsmOp>(opInst)) {
+    // TODO: refactor function type creation which usually occurs in std-LLVM
+    // conversion.
+    SmallVector<LLVM::LLVMType, 8> operandTypes;
+    operandTypes.reserve(inlineAsmOp.operands().size());
+    for (auto t : inlineAsmOp.operands().getTypes())
+      operandTypes.push_back(t.cast<LLVM::LLVMType>());
+
+    LLVM::LLVMType resultType;
+    if (inlineAsmOp.getNumResults() == 0) {
+      resultType = LLVM::LLVMType::getVoidTy(mlirModule->getContext());
+    } else if (inlineAsmOp.getNumResults() == 1) {
+      resultType = inlineAsmOp.getResultTypes()[0].cast<LLVM::LLVMType>();
+    } else {
+      SmallVector<LLVM::LLVMType, 8> unpackedResultTypes;
+      unpackedResultTypes.reserve(inlineAsmOp.getResultTypes().size());
+      for (auto t : inlineAsmOp.getResultTypes())
+        unpackedResultTypes.push_back(t.cast<LLVM::LLVMType>());
+      resultType = LLVM::LLVMType::getStructTy(mlirModule->getContext(),
+                                               unpackedResultTypes);
+    }
+
+    auto ft = LLVM::LLVMType::getFunctionTy(resultType, operandTypes,
+                                            /*isVarArg=*/false);
+    auto *IA =
+        inlineAsmOp.asm_dialect().hasValue()
+            ? llvm::InlineAsm::get(
+                  static_cast<llvm::FunctionType *>(convertType(ft)),
+                  inlineAsmOp.asm_string(), inlineAsmOp.constraints(),
+                  inlineAsmOp.has_side_effects(), inlineAsmOp.is_align_stack(),
+                  convertAsmDialectToLLVM(*inlineAsmOp.asm_dialect()))
+            : llvm::InlineAsm::get(
+                  static_cast<llvm::FunctionType *>(convertType(ft)),
+                  inlineAsmOp.asm_string(), inlineAsmOp.constraints(),
+                  inlineAsmOp.has_side_effects(), inlineAsmOp.is_align_stack());
+    llvm::Value *result =
+        builder.CreateCall(IA, lookupValues(inlineAsmOp.operands()));
+    if (opInst.getNumResults() != 0)
+      valueMapping[opInst.getResult(0)] = result;
+    return success();
+  }
+
   if (auto invOp = dyn_cast<LLVM::InvokeOp>(opInst)) {
     auto operands = lookupValues(opInst.getOperands());
     ArrayRef<llvm::Value *> operandsRef(operands);
     if (auto attr = opInst.getAttrOfType<FlatSymbolRefAttr>("callee")) {
       builder.CreateInvoke(functionMapping.lookup(attr.getValue()),
                            blockMapping[invOp.getSuccessor(0)],
                            blockMapping[invOp.getSuccessor(1)], operandsRef);
     } else {

mlir/test/Dialect/LLVMIR/roundtrip.mlir

func @useFenceInst() {
   // CHECK:  syncscope("agent") seq_cst
   llvm.fence syncscope("agent") seq_cst
   // CHECK:  seq_cst
   llvm.fence syncscope("") seq_cst
   // CHECK:  release
   llvm.fence release
   return
 }
+
+// CHECK-LABEL: @useInlineAsm
+llvm.func @useInlineAsm(%arg0: !llvm.i32) {
+  //      CHECK:  llvm.inline_asm {{.*}} (!llvm.i32) -> !llvm.i8
+  %0 = llvm.inline_asm "bswap $0", "=r,r" %arg0 : (!llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  llvm.inline_asm {{.*}} (!llvm.i32, !llvm.i32) -> !llvm.i8
+  %1 = llvm.inline_asm "foo", "bar" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  llvm.inline_asm has_side_effects {{.*}} (!llvm.i32, !llvm.i32) -> !llvm.i8
+  %2 = llvm.inline_asm has_side_effects "foo", "bar" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  llvm.inline_asm is_align_stack {{.*}} (!llvm.i32, !llvm.i32) -> !llvm.i8
+  %3 = llvm.inline_asm is_align_stack "foo", "bar" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  llvm.inline_asm asm_dialect = 1 {{.*}} (!llvm.i32, !llvm.i32) -> !llvm.i8
+  %4 = llvm.inline_asm asm_dialect = 1 "foo", "bar" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  llvm.return
+}

mlir/test/Target/llvmir.mlir

 
 // CHECK-NOT: "CodeView", i32 1
 module attributes {llvm.target_triple = "aarch64-linux-android"} {}
 
 // -----
 
 // CHECK-NOT: "CodeView", i32 1
 module attributes {} {}
+
+// -----
+
+// CHECK-LABEL: @useInlineAsm
+llvm.func @useInlineAsm(%arg0: !llvm.i32) {
+  // Constraints string is checked at LLVM InlineAsm instruction construction time.
+  // So we can't just use "bar" everywhere, number of in/out arguments has to match.
+
+  // CHECK-NEXT:  call void asm "foo", "r"(i32 {{.*}}), !dbg !7
+  llvm.inline_asm "foo", "r" %arg0 : (!llvm.i32) -> ()
+
+  // CHECK-NEXT:  call i8 asm "foo", "=r,r"(i32 {{.*}}), !dbg !9
+  %0 = llvm.inline_asm "foo", "=r,r" %arg0 : (!llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  call i8 asm "foo", "=r,r,r"(i32 {{.*}}, i32 {{.*}}), !dbg !10
+  %1 = llvm.inline_asm "foo", "=r,r,r" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  call i8 asm sideeffect "foo", "=r,r,r"(i32 {{.*}}, i32 {{.*}}), !dbg !11
+  %2 = llvm.inline_asm has_side_effects "foo", "=r,r,r" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  call i8 asm alignstack "foo", "=r,r,r"(i32 {{.*}}, i32 {{.*}}), !dbg !12
+  %3 = llvm.inline_asm is_align_stack "foo", "=r,r,r" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  call i8 asm inteldialect "foo", "=r,r,r"(i32 {{.*}}, i32 {{.*}}), !dbg !13
+  %4 = llvm.inline_asm asm_dialect = 1 "foo", "=r,r,r" %arg0, %arg0 : (!llvm.i32, !llvm.i32) -> !llvm.i8
+
+  // CHECK-NEXT:  call { i8, i8 } asm "foo", "=r,=r,r"(i32 {{.*}}), !dbg !14
+  %5 = llvm.inline_asm "foo", "=r,=r,r" %arg0 : (!llvm.i32) -> !llvm.struct<(i8, i8)>
+
+  llvm.return
+}
+

mlir/tools/mlir-cpu-runner/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Core
   Support
   nativecodegen
   )
 
 add_llvm_tool(mlir-cpu-runner
   mlir-cpu-runner.cpp
   )
 llvm_update_compile_flags(mlir-cpu-runner)
 get_property(dialect_libs GLOBAL PROPERTY MLIR_DIALECT_LIBS)
 target_link_libraries(mlir-cpu-runner PRIVATE
   ${dialect_libs}
+  LLVMAsmParser
+  LLVMX86AsmParser
   MLIRAnalysis
   MLIREDSC
   MLIRExecutionEngine
   MLIRIR
   MLIRJitRunner
   MLIRLLVMIR
   MLIRParser
   MLIRTargetLLVMIR
   MLIRSupport
   )

mlir/tools/mlir-cpu-runner/mlir-cpu-runner.cpp

 #include "mlir/InitAllDialects.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/TargetSelect.h"
 
 int main(int argc, char **argv) {
   llvm::InitLLVM y(argc, argv);
   llvm::InitializeNativeTarget();
   llvm::InitializeNativeTargetAsmPrinter();
+  llvm::InitializeNativeTargetAsmParser();
   mlir::initializeLLVMPasses();
 
   return mlir::JitRunnerMain(argc, argv);
 }