Page MenuHomePhabricator

[mlir][LLVM] Add memref dialect cast
AcceptedPublic

Authored by kdobros on Aug 5 2020, 9:29 AM.

Details

Summary

This change adds support for casting memref from/to memref descriptor
struct via llvm.mlir.dialect_cast op.
To provide verification for this operation rewriting verify method was
needed, which strengthens previous checks by additionally checking
input-output types compatibility.

Such cast is useful when interfacing with code that doesn't need all
information that memref contains, for example requiring only bitcasted
pointer to data.

Diff Detail

Unit TestsFailed

TimeTest
20 mslinux > MLIR.Conversion/StandardToLLVM::convert-to-llvmir_32bit.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-opt -convert-std-to-llvm='index-bitwidth=32' /mnt/disks/ssd0/agent/llvm-project/mlir/test/Conversion/StandardToLLVM/convert-to-llvmir_32bit.mlir -split-input-file | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/mlir/test/Conversion/StandardToLLVM/convert-to-llvmir_32bit.mlir
30 mslinux > MLIR.Conversion/StandardToLLVM::convert-to-llvmir_64bit.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/mlir-opt -convert-std-to-llvm /mnt/disks/ssd0/agent/llvm-project/mlir/test/Conversion/StandardToLLVM/convert-to-llvmir_64bit.mlir -split-input-file | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/mlir/test/Conversion/StandardToLLVM/convert-to-llvmir_64bit.mlir
30 mswindows > MLIR.Conversion/StandardToLLVM::convert-to-llvmir_32bit.mlir
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\mlir-opt.exe -convert-std-to-llvm='index-bitwidth=32' C:\ws\w64\llvm-project\premerge-checks\mlir\test\Conversion\StandardToLLVM\convert-to-llvmir_32bit.mlir -split-input-file | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\mlir\test\Conversion\StandardToLLVM\convert-to-llvmir_32bit.mlir
50 mswindows > MLIR.Conversion/StandardToLLVM::convert-to-llvmir_64bit.mlir
Script: -- : 'RUN: at line 1'; c:\ws\w64\llvm-project\premerge-checks\build\bin\mlir-opt.exe -convert-std-to-llvm C:\ws\w64\llvm-project\premerge-checks\mlir\test\Conversion\StandardToLLVM\convert-to-llvmir_64bit.mlir -split-input-file | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\mlir\test\Conversion\StandardToLLVM\convert-to-llvmir_64bit.mlir

Event Timeline

kdobros created this revision.Aug 5 2020, 9:29 AM
kdobros requested review of this revision.Aug 5 2020, 9:29 AM
ftynse requested changes to this revision.Aug 5 2020, 11:46 AM

Thanks!

Have you considered leveraging the TypeConverter instead of re-implementing half of the conversion rules in the verifier? It sounds like, in most cases, it should be sufficient to verify that LLVM::TypeConverter(...).convert(stdType) == llvmType, index type being the notable exception.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
991

Please don't use auto unless it improves readability (e.g., the RHS has dyn_cast or is a constructor/factory).

994

Please terminate sentences with dots in all comments.

1003

Please provide tests for all user-visible error messages.

1012–1014

This looks like it supports casting f16 to !llvm.double, which isn't desirable. Consider using stdType.isF16()-style functions for stricter checks.

1015

Should we also check the bitwidth of the integer?

1017

Nit: drop trivial braces

1024

Don't use else after return.

1042

Does this handle the special case of function calling conventions? The default convention expands the memref descriptor into multiple types, and we also have the "bare-pointer" convention that uses a plain pointer intsead.

1071

Consider inverting the condition and returning early to decrease horizontal indentation.

mlir/test/Conversion/StandardToLLVM/convert-to-llvmir_32bit.mlir
1

I don't understand the purpose of these tests. The code only tests the verifier of an op, so I would expect "roundtrip" tests that parse and print back accepted versions of the op and "invalid" tests that check for errors produced by the verifier.

This revision now requires changes to proceed.Aug 5 2020, 11:46 AM
kdobros updated this revision to Diff 283901.Aug 7 2020, 7:11 AM
kdobros retitled this revision from [mlir][StandardToLLVM] Add memref dialect cast to [mlir][LLVM] Add memref dialect cast.

Addressed review comments.
Rewrote tests to check only the verifier - roundtrip.mlir and invalid.mlir.
Reduced number of error messages.

kdobros marked 8 inline comments as done.Aug 7 2020, 7:28 AM

Thanks for the review.

Have you considered leveraging the TypeConverter instead of re-implementing half of the conversion rules in the verifier? It sounds like, in most cases, it should be sufficient to verify that LLVM::TypeConverter(...).convert(stdType) == llvmType, index type being the notable exception.

I have thought about this, but I'm not a fan of making verifier depend on conversion, and also working around index type would be tricky,
I think best way would be to move some type conversions, that don't depend on convention, into dialect and reuse them in both StandardToLLVM and in verifier.
This however seems like a little tricky and somewhat unrelated, so I've added TODO mentioning this.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1015

Seems reasonable if we're checking FloatType compatibility.

1042

Sorry, I'm not aware where calling convention could interact with dialect cast.
From what I understand irrespective of calling convention all function arguments are immediately packed back into memref descriptor struct.
Only place I guess pointer could be casted to memref is when moving from LLVM to standard and from LLVM comes only this pointer.
For now such case can be only supported with manually creating correct descriptor.
Automatically converting something like this would require adding rewrite pattern (or maybe legalization), so verifier rejects it, at least for now.
Could you maybe explain, if I'm misunderstanding something?

ftynse accepted this revision.Aug 7 2020, 8:31 AM

I have thought about this, but I'm not a fan of making verifier depend on conversion, and also working around index type would be tricky,

And I am not a fan of duplicating the logic :) Working around index (and calling convention) should be easier when we have a data layout attached to the module, it will be possible to just get the proper index size from the layout and use it.

I think best way would be to move some type conversions, that don't depend on convention, into dialect and reuse them in both StandardToLLVM and in verifier.

I disagree. The reason why we have lib/Conversion/AToB is that conversions depend on both A and B and we did not want to have dialect B depend on A. Putting type conversions in the dialect is the exact opposite of this goal. (Well, to be honest, standard types aren't a part of the standard dialect, but that's just a quirk of standard stuff). There aren't many other cases where type casts are inserted, and the LLVM dialect has always been the trailblazer in the area, but we need to find a cleaner way. We put the "cast" op into the LLVM because we did not want to have it in standard as it could be mistaken for some magic "cast any type to any other type" op with no verification. Maybe we should consider a separate "cast" dialect with pluggable rules, or a separate dialect that can depend on both LLVM and Standard, but not making LLVM depend on Standard (or any other dialect that may lower into LLVM, e.g. Linalg also has type conversions).

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1022

It's valid but useless, should we add a canonicalization pattern that drops such casts? (not for this commit)

1042

I don't remember offhand how the casts and calling convention interact... The best thing to do is arguably try to run the newly introduced tests with both calling conventions and check that they still pass.

1077–1079

can we do something like

op.emitOpError("incorrect cast").attachNote("casting is supported for integers and floats, vectors and memrefs thereof");

that is more concise and decouples the error message from the additional explanation?

This revision is now accepted and ready to land.Aug 7 2020, 8:31 AM
kdobros updated this revision to Diff 284945.Aug 11 2020, 6:24 PM
kdobros marked an inline comment as done.

Split emitted error into error and note

kdobros marked an inline comment as done.Aug 11 2020, 6:58 PM

I think best way would be to move some type conversions, that don't depend on convention, into dialect and reuse them in both StandardToLLVM and in verifier.

I disagree. The reason why we have lib/Conversion/AToB is that conversions depend on both A and B and we did not want to have dialect B depend on A. Putting type conversions in the dialect is the exact opposite of this goal. (Well, to be honest, standard types aren't a part of the standard dialect, but that's just a quirk of standard stuff). There aren't many other cases where type casts are inserted, and the LLVM dialect has always been the trailblazer in the area, but we need to find a cleaner way. We put the "cast" op into the LLVM because we did not want to have it in standard as it could be mistaken for some magic "cast any type to any other type" op with no verification. Maybe we should consider a separate "cast" dialect with pluggable rules, or a separate dialect that can depend on both LLVM and Standard, but not making LLVM depend on Standard (or any other dialect that may lower into LLVM, e.g. Linalg also has type conversions).

Sure, I think it's still not clear to me where core IR ends and where Standard dialect begins, especially for types.
To me making verifier of dialect depend on conversion logic also breaks this separation goal, just in more roundabout way.
IMHO separate cast dialect with pluggable rules makes most sense. Biggest potential advantage here is that it could allow for casts between dialects that don't know about each other by looking for chain of cast rules that would satisfy input/output types. It could also allow to limit explosion of rules by using some pivot dialect like Standard. However implementation seems well above my current familiarity with MLIR.


I think I have addressed all comments.
If you think the change still looks good, could you maybe commit on my behalf? I don't have commit access.
My name and mail: Konrad Dobros <konrad.dobros@intel.com>

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
1022

Sure, I actually though there already was such canonicalization since I saw that dialect casts with matching types are removed in lowering from standard.

1042

I looked at that first version where separate tests for Standard to LLVM lowering were added.
I saw no difference between calling conventions and memrefs being packed to descriptor struct immediately.

1077–1079

Sure, changed.
I haven't noticed that there is this attachNote method.