This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by imaihal on May 28 2020, 12:25 AM.

Details

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

Diff Detail

Event Timeline

imaihal created this revision.May 28 2020, 12:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2020, 12:25 AM
imaihal updated this revision to Diff 266760.May 28 2020, 12:35 AM

Fixed "No newline at end of file"

imaihal updated this revision to Diff 266869.May 28 2020, 7:16 AM

Rebased with master

imaihal edited the summary of this revision. (Show Details)May 28 2020, 10:10 PM
imaihal edited the summary of this revision. (Show Details)
imaihal edited the summary of this revision. (Show Details)
imaihal added a comment.EditedJun 2 2020, 5:45 PM

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

Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2020, 5:45 PM
imaihal added a comment.EditedJun 29 2020, 8:17 PM

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

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?

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.

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.

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.

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

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.

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 added a comment.EditedSep 8 2020, 9:38 PM

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.

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.

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.

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

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 added a comment.EditedSep 14 2020, 5:48 PM

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 updated this revision to Diff 299073.Oct 19 2020, 8:52 AM

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.

imaihal retitled this revision from [mlir] Added big endian version of "dense-elements-hex.mlir" to [mlir] Convert raw data in dense element attributes into big-endian format..Oct 19 2020, 8:54 AM
imaihal edited the summary of this revision. (Show Details)
mehdi_amini added inline comments.Oct 19 2020, 8:26 PM
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 updated this revision to Diff 299349.Oct 20 2020, 6:21 AM
imaihal retitled this revision from [mlir] Convert raw data in dense element attributes into big-endian format. to [mlir] Convert raw data in dense element attributes into big-endian format..

Reflected @mehdi_amini's comments.

imaihal marked 4 inline comments as done.Oct 20 2020, 6:29 AM
imaihal added inline comments.
mlir/lib/Support/EndianUtilities.cpp
54 ↗(On Diff #299073)

const ulittle16_t * works. Thanks!

59 ↗(On Diff #299073)

Sorry, it works.

imaihal marked 2 inline comments as done.Oct 20 2020, 6:30 AM

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

imaihal retitled this revision from [mlir] Convert raw data in dense element attributes into big-endian format. to [mlir] Convert raw data in dense element attributes for big-endian machines..Oct 20 2020, 6:48 AM
imaihal edited the summary of this revision. (Show Details)Oct 20 2020, 7:00 AM
rriddle requested changes to this revision.Oct 20 2020, 10:24 AM
rriddle added inline comments.
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/.

This revision now requires changes to proceed.Oct 20 2020, 10:24 AM
imaihal updated this revision to Diff 299554.Oct 20 2020, 10:48 PM

Moved the convEndianBE into DenseIntOrFPElementsAttr class and removed EndianUtilities.

imaihal updated this revision to Diff 299557.Oct 20 2020, 11:13 PM

Removed unnecessary headers.

imaihal marked an inline comment as done.Oct 20 2020, 11:18 PM
imaihal added inline comments.
mlir/lib/Support/EndianUtilities.cpp
44 ↗(On Diff #299349)

Thanks for your review. I moved it to DenseIntOrFPElementsAttr .

rriddle added inline comments.Oct 21 2020, 6:17 PM
mlir/include/mlir/IR/Attributes.h
1145

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

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

1512–1514

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

Can you move these inside of convEndianBE?

mlir/lib/Parser/AttributeParser.cpp
700

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

705

Same comments here.

imaihal updated this revision to Diff 300668.Oct 26 2020, 7:41 AM
imaihal marked an inline comment as done.

Reflected comments and rebased.

imaihal marked 5 inline comments as done.Oct 26 2020, 7:49 AM
imaihal added inline comments.
mlir/lib/IR/AsmPrinter.cpp
1509

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.

imaihal marked an inline comment as done.Oct 26 2020, 5:30 PM
rriddle added inline comments.Oct 26 2020, 5:45 PM
mlir/include/mlir/IR/Attributes.h
1143

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

mlir/lib/IR/AsmPrinter.cpp
1514

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

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

1154

Is this check necessary anymore?

1156

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

1162

Drop this empty return.

1177

Can you drop the DenseIntOrFPElementsAttr:: here?

mlir/lib/Parser/AttributeParser.cpp
680

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

725

nit: Drop the else after return here.

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

mlir/include/mlir/IR/Attributes.h
1143

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
1514

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 accepted this revision.Oct 26 2020, 7:06 PM

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

mlir/include/mlir/IR/Attributes.h
1143

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
1514

Okay, that kind of makes sense now.

This revision is now accepted and ready to land.Oct 26 2020, 7:06 PM
imaihal updated this revision to Diff 300988.Oct 27 2020, 7:44 AM
imaihal marked 11 inline comments as done.

Reflect comments.

imaihal updated this revision to Diff 301166.Oct 27 2020, 7:45 PM

Avoid warning in assert().

imaihal updated this revision to Diff 301169.Oct 27 2020, 8:16 PM

Avoid warning message.

imaihal added inline comments.Oct 27 2020, 8:57 PM
mlir/lib/IR/Attributes.cpp
1128–1129

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

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