mlir icon indicating copy to clipboard operation
mlir copied to clipboard

mlir-translate error + missing location pointing

Open bondhugula opened this issue 6 years ago • 11 comments

On the appended MLIR, mlir-translate produces this error.

$ mlir-translate -mlir-to-llvmir /tmp/single.mlir loc("/tmp/single.mlir":1:6): error: non-LLVM function argument type

I see two issues:

  1. On a minor note, the error message is missing reproduction and pinpointing to the source line being referred to.
  2. It's not clear what the error is about. It looks like it's pointing to the '@' sigil (the sixth character), but that's not a function argument. Is this due to something missing with memref type lowering? The doc at https://github.com/tensorflow/mlir/blob/master/g3doc/ConversionToLLVMDialect.md doesn't say so.

func @simple_func(%arg0: memref<8x8xvector<64xf32>>, %arg1: memref<8x8xvector<64xf32>>, %arg2: memref<8x8xvector<64xf32>>) -> memref<8x8xvector<64xf32>> { affine.for %i = 0 to 256 { affine.for %j = 0 to 256 { affine.for %k = 0 to 250 { %l = load %arg0[%i, %k] : memref<8x8xvector<64xf32>> %r = load %arg1[%k, %j] : memref<8x8xvector<64xf32>> %o = load %arg2[%i, %j] : memref<8x8xvector<64xf32>> %m = mulf %l, %r : vector<64xf32> %a = addf %o, %m : vector<64xf32> store %a, %arg2[%i, %j] : memref<8x8xvector<64xf32>> } } } return %arg2 : memref<8x8xvector<64xf32>> }

bondhugula avatar Apr 22 '19 02:04 bondhugula

On the other hand, doing the translation with mlir-opt -convert-to-llvmir doesn't have this issue. But it yields:

$ mlir-opt -convert-to-llvmir /tmp/single.mlir /tmp/single.mlir:2:3: error: 'affine.for' op expected body to have a single index argument for the induction variable affine.for %i = 0 to 256 {

I think the error message here is misleading (or perhaps a bug). The following however works since these 'for' op's are lowered away.

$ mlir-opt -lower-affine -convert-to-llvmir /tmp/single.mlir | mlir-translate -mlir-to-llvmir

Is mlir-translate missing documentation or is it out of date? It doesn't appear at https://github.com/tensorflow/mlir/blob/master/g3doc/ConversionToLLVMDialect.md and it's not clear what the difference b/w mlir-opt's -convert-to-llvmir and mlir-translate's -mlir-to-llvmir is.

bondhugula avatar Apr 22 '19 02:04 bondhugula

mlir-translate is intended to translate MLIR to other IRs. Some of the translations require MLIR to only have certain dialects, in particular translation to LLVM IR requires MLIR input to only use the LLVM IR dialect. Hence the complaint about non-LLVM function argument types. None of them belongs to the LLVM dialect.

mlir-opt performs MLIR-to-MLIR transformations, including dialect conversions. The document you referenced is about converting to the dialect, not to the LLVM IR proper. The conversion is implemented for a subset of standard dialect. You need to lower away all other dialects before calling it. There is currently no way of triggering a chain of conversions that would somehow know that there is a pass that lowers away loops.

ftynse avatar Apr 24 '19 16:04 ftynse

Right. This is behaving correctly. mlir-translate is just supposed to do the last step of translation (format conversion) it does not do lowering.

lattner avatar Apr 24 '19 23:04 lattner

I see - thanks. In that case, shouldn't mlir-opt's '-convert-to-llvmir' be renamed - since it's converting to LLVM dialect as opposed to LLVM IR? Perhaps to '-convert-to-llvm'?

bondhugula avatar Apr 25 '19 01:04 bondhugula

What about -lower-to-llvm or -llvm-lowering?

Even if issuing an error is expected here, the error messages should really be improved here, we should keep this bug open to track fixing this.

joker-eph avatar Apr 25 '19 03:04 joker-eph

+1 for renaming to "lower-to-llvm" and improving the error messages!

lattner avatar Apr 25 '19 05:04 lattner

Yes, even I wanted to point out that renaming sometime back. "lower-to-llvm" makes much more sense to me too.

madhur13490 avatar Apr 27 '19 09:04 madhur13490

The renaming was done in 822e3fc877e89dd7dc73d3018accdda6ea30b537

joker-eph avatar May 09 '19 03:05 joker-eph

Thanks. The only thing remaining on this issue is this error message that probably has to be improved.

$ mlir-opt -lower-to-llvm /tmp/single.mlir /tmp/single.mlir:2:3: error: 'affine.for' op expected body to have a single index argument for the induction variable affine.for %i = 0 to 256 {

This should have ideally been a "can't lower 'affine.for' op".

bondhugula avatar May 11 '19 06:05 bondhugula

Issues referenced here have been addressed, pending one is a request to rename which could best be done by way of a PR & discussion there, so closing this issue.

jpienaar avatar Jul 09 '19 16:07 jpienaar

Actually the rename has been done, the remaining issue is about the lowering error message being confusing.

joker-eph avatar Jul 10 '19 22:07 joker-eph