This is an archive of the discontinued LLVM Phabricator instance.

[mlir][LLVMIR] Add support for InlineAsmOp
ClosedPublic

Authored by nicolasvasilache on Nov 26 2020, 3:15 AM.

Details

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.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache requested review of this revision.Nov 26 2020, 3:15 AM
rriddle added inline comments.Nov 26 2020, 3:17 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
1221

Please use a multi-line string instead:

[{ ... }]

mehdi_amini added inline comments.Nov 26 2020, 5:35 PM
mlir/test/Dialect/Neon/roundtrip.mlir
25 ↗(On Diff #307811)

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

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

nicolasvasilache marked 2 inline comments as done.Nov 27 2020, 1:17 AM
nicolasvasilache added inline comments.
mlir/test/Dialect/Neon/roundtrip.mlir
25 ↗(On Diff #307811)

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

ftynse requested changes to this revision.Nov 27 2020, 2:13 AM
ftynse added inline comments.
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

This revision now requires changes to proceed.Nov 27 2020, 2:13 AM
nicolasvasilache marked 5 inline comments as done.Nov 27 2020, 8:49 AM
nicolasvasilache added inline comments.
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 marked 2 inline comments as done.

Address comments.

ftynse accepted this revision.Nov 27 2020, 9:24 AM
ftynse added inline comments.
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.

This revision is now accepted and ready to land.Nov 27 2020, 9:24 AM

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 added inline comments.Nov 27 2020, 11:28 AM
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.

Address comments.

nicolasvasilache edited the summary of this revision. (Show Details)Nov 30 2020, 12:12 AM

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.

This revision was automatically updated to reflect the committed changes.
rovka added a subscriber: rovka.Nov 30 2020, 6:31 AM

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,

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 ?

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.

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

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).