[mlir] Convert raw data in dense element attributes for big-endian machines.

[imaihal]

Summary

This patch fixes a bug 46091

Raw data for the dense-element attribute is written in little endian (LE) format.
This commit converts the format to big endian (BE) in ʻAttribute Parser` on the
BE machine. Also, when outputting on a BE machine, the BE format is converted
to LE in "AsmPrinter".

[imaihal]

Fixed "No newline at end of file"

[imaihal]

Rebased with master

[imaihal]

Could anyone be a reviewer of this patch? This patch is about one of the MLIR test (dense-elements-hex.mlir).

[imaihal]

@rriddle Hi, is it possible to give me your comments about this patch? I'm asking because you were a reviewer of the previous patch including this file.

[imaihal]

Hi, I'm looking for reviewers for this patch. This patch is old. So, I need to update, but I would like to hear your comments about how we should pass this test on big-endian machines. Since current test includes HEX of little endian, this test fails on big-endian machines.
@jpienaar, @ftynse, @mehdi_amini, @rriddle @Smit You were reviewers for patches including this test before. Could you give me your comments?

[AlexEichenberger]

Rephrasing the question.

The original mlir/test/IR/dense-elements-hex.mlir file assumes that test machines are Little Endian. IBM has big endian machines, and we would like to have a clean run on Big Endian machines as well.

What is your preferred way to handle the situation?

1. Two different directories with endian-specific files,
2. two different files in the current directory, or
3. is there some ways to run the same file but with different checks?

We will implement your preferred approach. Thanks for the help.

[mehdi_amini]

Could the HEX dump be made independent of the platform? Having the printed format dumped on a BE loadable on a LE and vice-versa would be nicer I think.

[imaihal]

In D80695#2251361, @mehdi_amini wrote:

Could the HEX dump be made independent of the platform? Having the printed format dumped on a BE loadable on a LE and vice-versa would be nicer I think.

Thanks for your suggestion. Test code that does not depend on endianness is better. I'm checking other test codes in LLVM. If you know such an example, please let me know. That should be helpful.

[mehdi_amini]

I think looking for where the endian::read method is used in the codebase can yield some inspiration, for example: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Remarks/YAMLRemarkParser.cpp#L76

[imaihal]

Thanks. I checked the source code. The dense attribute with hex data is parsed here https://github.com/llvm/llvm-project/blob/master/mlir/lib/Parser/AttributeParser.cpp#L682-L684
If we can assume the hex data is always little-endian, it can be converted here, but we can't assume that. So, my understanding is that the hex data of dense-elements-hex.mlir should be generated depending on the endianness. Is this possible? I'm looking for similar examples.

[mehdi_amini]

If we can assume the hex data is always little-endian, it can be converted here, but we can't assume that.

Why? We control the printer and parser, don't we?

[imaihal]

In D80695#2262458, @mehdi_amini wrote:

If we can assume the hex data is always little-endian, it can be converted here, but we can't assume that.

Why? We control the printer and parser, don't we?

The hex of current dense-elements-hex.mlir is LE. So, we can pass the test by controlling the printer and parser. ( LE to BE in the parser, and BE to LE in the printer)
If we can force users to write LE in the attribute, there is no problem. However, users may write the hex of BE in the attributes in BE machines.
In that case, the parser and printer should not convert the hex, but since the parser and printer don't know whether the hex in the attribute is BE or LE, I think they can not process correctly.

[mehdi_amini]

If we can force users to write LE in the attribute, there is no problem. However, users may write the hex of BE in the attributes in BE machines.

I don't understand what you're referring to right now: can you elaborate how a user would write the hex in BE?
I expect a users to form the data in memory in BE on a BE machine, and then the printer will print in LE form always.

[AlexEichenberger]

Just my two cents; LE vs BE typically gives problem when a program read data with one data size, and write data with another size. So if the reader in the dense attributes respect the type size, namely read and write the data using the same format, and then generate the hex directly from the register content, then there are no issues with endianness.

From past optimizations, there are case when you want to write 64 bits at once, but later want to ready only 8 bits... then endianness is an issue as the program needs to know where to find the right 8 among the 64 in memory. But I don't think this is the case here, as the dense attributes are clearly typed and I see no reason not to read & write using the same one.

[imaihal]

In D80695#2262569, @mehdi_amini wrote:

I don't understand what you're referring to right now: can you elaborate how a user would write the hex in BE?
I expect a users to form the data in memory in BE on a BE machine, and then the printer will print in LE form always.

I thought native byte order should be written in dense.attr. (Users write BE HEX when they use BE machine) If this is not preferable, I think it would be better to describe it in MLIR specification.

Following link is just an example of ONNX.
https://github.com/onnx/onnx/blob/3368834cf0b1f0ab9838cf6bdf78a27299d08187/onnx/onnx.in.proto#L538-L539

[mehdi_amini]

In D80695#2265193, @imaihal wrote:

I thought native byte order should be written in dense.attr.

Well that's my point all along: let's not do that and make the serialization/deserialization cross-platform.

Users write BE HEX

Nit: users don't write HEX I believe, the MLIR printer does for them, and we control the printer I believe, at least for the standard types.

[imaihal]

In D80695#2266442, @mehdi_amini wrote:

In D80695#2265193, @imaihal wrote:

I thought native byte order should be written in dense.attr.

Well that's my point all along: let's not do that and make the serialization/deserialization cross-platform.

OK. I will implement the conversion, assuming that HEX in dense.attr is always LE. Thanks!

[imaihal]

Implemented the conversion from LE to BE in AttributeParser and from BE to LE in AsmPrinter
, assuming that HEX in dense.attr is always LE.

[mehdi_amini]

mlir/lib/Support/EndianUtilities.cpp

44 ↗

(On Diff #299073)

I expect a MutableArrayRef here?

52 ↗

(On Diff #299073)

Can you assert(numElements*storageBitWidth == inRawData.size()) and inRawData.size() <= outRawData.size() ?

54 ↗

(On Diff #299073)

It does not work with const ulittle16_t * ?

If so, can you extract the const_cast outside of the sequence of if (and make it a switch?):

char *rawDataBegin = const_cast<char *>(inRawData.begin());
switch (storageBitWidth) {
  case 16: {
    ulittle16_t *inRawDataPos =  reinterpret_cast<ulittle16_t *>(rawDataBegin);
    uint16_t *outDataPos  = reinterpret_cast<uint16_t *>(outRawData.begin());
    for (unsigned i = 0, e = numElements; i < e; ++i)
      std::copy_n(inRawDataPos + i, 1, outDataPos + i);
  }
  ...

59 ↗

(On Diff #299073)

It isn't clear to me right now why you need a loop instead of using the second argument of std::copy_n?

std::copy_n(inRawDataPos, numElements, outDataPos);

[imaihal]

Reflected @mehdi_amini's comments.

[imaihal]

mlir/lib/Support/EndianUtilities.cpp

54 ↗

(On Diff #299073)

const ulittle16_t * works. Thanks!

59 ↗

(On Diff #299073)

Sorry, it works.

[imaihal]

@mehdi_amini Thanks for your review! I reflected all of your comments.

[rriddle]

mlir/lib/Support/EndianUtilities.cpp

44 ↗

(On Diff #299349)

Can you just make this a utility method on DenseIntOrFpElementsAttr itself? This removes the duplicated code, and also removes the need to break library layering by having Support/ depend on IR/.

[imaihal]

Moved the convEndianBE into DenseIntOrFPElementsAttr class and removed EndianUtilities.

[imaihal]

Removed unnecessary headers.

[imaihal]

mlir/lib/Support/EndianUtilities.cpp

44 ↗

(On Diff #299349)

Thanks for your review. I moved it to DenseIntOrFPElementsAttr .

[rriddle]

mlir/include/mlir/IR/Attributes.h

1145 ↗

(On Diff #299557)

Can you rename the function to something a bit longer and more descriptive? I don't expect this function to be called very often, if ever by users, so a longer function name isn't that much of a detriment.

mlir/lib/IR/AsmPrinter.cpp

1509 ↗

(On Diff #299557)

If this machine is already little-endian, can we remove the redundant copy?

1512–1514 ↗

(On Diff #299557)

This doesn't look correct, you aren't initializing the SmallVector meaning that data is not guaranteed to be a valid storage pointer. You also don't need to manually construct a MutableArrayRef, it should be implicitly constructible from SmallVector.

mlir/lib/IR/Attributes.cpp

24 ↗

(On Diff #299557)

Can you move these inside of convEndianBE?

mlir/lib/Parser/AttributeParser.cpp

705 ↗

(On Diff #299557)

If this machine is already little-endian, can we remove the redundant copy?

710 ↗

(On Diff #299557)

Same comments here.

[imaihal]

Reflected comments and rebased.

[imaihal]

mlir/lib/IR/AsmPrinter.cpp

1509 ↗

(On Diff #299557)

I couldn't find good way without checking system_endianness to avoid redundant copy for little endian. Little endian code is the same with original one.

[rriddle]

mlir/include/mlir/IR/Attributes.h

1143 ↗

(On Diff #300668)

Should we even support calling this if the machine is already LE?

mlir/lib/IR/AsmPrinter.cpp

1517 ↗

(On Diff #300668)

I may be confused right now, but doesn't convertEndianOfArrayRefForBEmachine just convert from little to big and not big to little?

mlir/lib/IR/Attributes.cpp

1123 ↗

(On Diff #300668)

Add an assert that the current machine is big endian here and in the function below?

1154 ↗

(On Diff #300668)

Is this check necessary anymore?

1156 ↗

(On Diff #300668)

nit: Just use size_t for i to remove the cast on nBytes. Or switch both to ssize_t.

1162 ↗

(On Diff #300668)

Drop this empty return.

1177 ↗

(On Diff #300668)

Can you drop the DenseIntOrFPElementsAttr:: here?

mlir/lib/Parser/AttributeParser.cpp

686 ↗

(On Diff #300668)

Can you just use these inline and drop the using? I don't think it saves much.

720 ↗

(On Diff #300668)

nit: Drop the else after return here.

[imaihal]

@rriddle Thanks for your comments. I answered some of the comments.

mlir/include/mlir/IR/Attributes.h

1143 ↗

(On Diff #300668)

We don't have to call this function in LE machines, but we can call this even in LE machines.
LE machine:
inRawData LE -> outRawData LE
BE machine:
inRawData LE -> outRawData BE

As you wrote before, there are redundant copy when we use this function in LE machine. To avoid this redundant copy, I think we need to check machine endianness as in this patch.

mlir/lib/IR/AsmPrinter.cpp

1517 ↗

(On Diff #300668)

This may be confusing, but this also converts big to little on BE machine.

copy_n copies inRawData(ulittle) to outRawData(uint). This copy assumes inRawData is LE format. So, this copy_n always converts endianness on BE machine even in actual inRawData is BE format.

Normally this is used when inRawData is LE format, but I reused to convert BE to LE.

[rriddle]

LGTM after resolving the remaining comments. Thanks for pushing on this!

mlir/include/mlir/IR/Attributes.h

1143 ↗

(On Diff #300668)

Yeah, that's why I'd be okay with asserting inside of these functions. It could prevent some accidental usages of these functions when they shouldn't be used. Dense data gets extremely large.

mlir/lib/IR/AsmPrinter.cpp

1517 ↗

(On Diff #300668)

Okay, that kind of makes sense now.

[imaihal]

Reflect comments.

[imaihal]

Rebased.

[imaihal]

Avoid warning in assert().

[imaihal]

Avoid warning message.

[imaihal]

mlir/lib/IR/Attributes.cpp

1128–1129 ↗

(On Diff #301169)

Inserted NOLINT to avoid warning messages in clang-tidy. The warning message suggested to use static_assert() here, but it is not appropriate here.

[imaihal]

@rriddle Thanks for your review. I don't have commit access. If you don't have other comments, could you commit this patch?

mlir/test/IR/BE/dense-elements-hex-be.mlir

 // RUN: mlir-opt -allow-unregistered-dialect %s -verify-diagnostics -split-input-file -mlir-print-elementsattrs-with-hex-if-larger=1 | FileCheck %s --check-prefix=HEX
 // RUN: mlir-opt -allow-unregistered-dialect %s -verify-diagnostics -split-input-file | FileCheck %s
 
-// HEX: dense<"0x00000000000024400000000000001440"> : tensor<2xf64>
+// HEX: dense<"0x40240000000000004014000000000000"> : tensor<2xf64>
 "foo.op"() {dense.attr = dense<[10.0, 5.0]> : tensor<2xf64>} : () -> ()
 
 // CHECK: dense<[1.000000e+01, 5.000000e+00]> : tensor<2xf64>
-"foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<2xf64>} : () -> ()
+"foo.op"() {dense.attr = dense<"0x40240000000000004014000000000000"> : tensor<2xf64>} : () -> ()
 
 // CHECK: dense<(1.000000e+01,5.000000e+00)> : tensor<2xcomplex<f64>>
-"foo.op"() {dense.attr = dense<"0x0000000000002440000000000000144000000000000024400000000000001440"> : tensor<2xcomplex<f64>>} : () -> ()
+"foo.op"() {dense.attr = dense<"0x4024000000000000401400000000000040240000000000004014000000000000"> : tensor<2xcomplex<f64>>} : () -> ()
 
 // CHECK: dense<[1.000000e+01, 5.000000e+00]> : tensor<2xbf16>
-"foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<2xbf16>} : () -> ()
+"foo.op"() {dense.attr = dense<"0x40240000000000004014000000000000"> : tensor<2xbf16>} : () -> ()
 
 // -----
 
 // expected-error@+1 {{elements hex string should start with '0x'}}
-"foo.op"() {dense.attr = dense<"00000000000024400000000000001440"> : tensor<2xf64>} : () -> ()
+"foo.op"() {dense.attr = dense<"40240000000000004014000000000000"> : tensor<2xf64>} : () -> ()
 
 // -----
 
 // expected-error@+1 {{elements hex string only contains hex digits}}
-"foo.op"() {dense.attr = dense<"0x0000000000002440000000000000144X"> : tensor<2xf64>} : () -> ()
+"foo.op"() {dense.attr = dense<"0x40240000000000004X14000000000000"> : tensor<2xf64>} : () -> ()
 
 // -----
 
 // expected-error@+1 {{elements hex data size is invalid for provided type}}
-"foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<4xf64>} : () -> ()
+"foo.op"() {dense.attr = dense<"0x40240000000000004014000000000000"> : tensor<4xf64>} : () -> ()

mlir/test/IR/BE/lit.local.cfg

+if not 's390x' in config.host_arch:
+  config.unsupported = True

mlir/test/IR/LE/dense-elements-hex-le.mlir

 // RUN: mlir-opt -allow-unregistered-dialect %s -verify-diagnostics -split-input-file -mlir-print-elementsattrs-with-hex-if-larger=1 | FileCheck %s --check-prefix=HEX
 // RUN: mlir-opt -allow-unregistered-dialect %s -verify-diagnostics -split-input-file | FileCheck %s
 
 // HEX: dense<"0x00000000000024400000000000001440"> : tensor<2xf64>
 "foo.op"() {dense.attr = dense<[10.0, 5.0]> : tensor<2xf64>} : () -> ()
 
 // CHECK: dense<[1.000000e+01, 5.000000e+00]> : tensor<2xf64>
 "foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<2xf64>} : () -> ()
 
 // CHECK: dense<(1.000000e+01,5.000000e+00)> : tensor<2xcomplex<f64>>
 "foo.op"() {dense.attr = dense<"0x0000000000002440000000000000144000000000000024400000000000001440"> : tensor<2xcomplex<f64>>} : () -> ()
 
 // CHECK: dense<[1.000000e+01, 5.000000e+00]> : tensor<2xbf16>
 "foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<2xbf16>} : () -> ()
 
 // -----
 
 // expected-error@+1 {{elements hex string should start with '0x'}}
 "foo.op"() {dense.attr = dense<"00000000000024400000000000001440"> : tensor<2xf64>} : () -> ()
 
 // -----
 
 // expected-error@+1 {{elements hex string only contains hex digits}}
 "foo.op"() {dense.attr = dense<"0x0000000000002440000000000000144X"> : tensor<2xf64>} : () -> ()
 
 // -----
 
 // expected-error@+1 {{elements hex data size is invalid for provided type}}
 "foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<4xf64>} : () -> ()

mlir/test/IR/LE/lit.local.cfg

+if 's390x' in config.host_arch:
+  config.unsupported = True

mlir/test/IR/dense-elements-hex.mlir

-// RUN: mlir-opt -allow-unregistered-dialect %s -verify-diagnostics -split-input-file -mlir-print-elementsattrs-with-hex-if-larger=1 | FileCheck %s --check-prefix=HEX
-// RUN: mlir-opt -allow-unregistered-dialect %s -verify-diagnostics -split-input-file | FileCheck %s
-
-// HEX: dense<"0x00000000000024400000000000001440"> : tensor<2xf64>
-"foo.op"() {dense.attr = dense<[10.0, 5.0]> : tensor<2xf64>} : () -> ()
-
-// CHECK: dense<[1.000000e+01, 5.000000e+00]> : tensor<2xf64>
-"foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<2xf64>} : () -> ()
-
-// CHECK: dense<(1.000000e+01,5.000000e+00)> : tensor<2xcomplex<f64>>
-"foo.op"() {dense.attr = dense<"0x0000000000002440000000000000144000000000000024400000000000001440"> : tensor<2xcomplex<f64>>} : () -> ()
-
-// CHECK: dense<[1.000000e+01, 5.000000e+00]> : tensor<2xbf16>
-"foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<2xbf16>} : () -> ()
-
-// -----
-
-// expected-error@+1 {{elements hex string should start with '0x'}}
-"foo.op"() {dense.attr = dense<"00000000000024400000000000001440"> : tensor<2xf64>} : () -> ()
-
-// -----
-
-// expected-error@+1 {{elements hex string only contains hex digits}}
-"foo.op"() {dense.attr = dense<"0x0000000000002440000000000000144X"> : tensor<2xf64>} : () -> ()
-
-// -----
-
-// expected-error@+1 {{elements hex data size is invalid for provided type}}
-"foo.op"() {dense.attr = dense<"0x00000000000024400000000000001440"> : tensor<4xf64>} : () -> ()