[mlir] Support big endian in DenseElementsAttr

[imaihal]

Summary

This std::copy_n copies 8 byte data (APInt raw data) by 1 byte from the
beginning of char array. This is no problem in little endian, but the
data is not copied correctly in big endian because the data should be
copied from the end of the char array.

• Example of 4 byte data (such as float32)

Little endian (First 4 bytes):
Address | 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08
Data | 0xcd 0xcc 0x8c 0x3f 0x00 0x00 0x00 0x00

Big endian (Last 4 bytes):
Address | 0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08
Data | 0x00 0x00 0x00 0x00 0x3f 0x8c 0xcc 0xcd

In general, when it copies N(N<8) byte data in big endian, the start
address should be incremented by (8 - N) bytes.
The original code has no problem when it includes 8 byte data(such as
double) even in big endian.

[rriddle]

Thanks for the patch!

mlir/lib/IR/Attributes.cpp

586–587

Can you extract this to a static helper above this function and add comments as to what is going on?

[imaihal]

[mlir] Create a static helper and add comments

[imaihal]

mlir/lib/IR/Attributes.cpp

586–587

Thanks for the review. I added a static helper and comments. Does this meet your request?

[rriddle]

Is there any way you can add a test for this?

mlir/lib/IR/Attributes.cpp

586–587

Can you add in the reinterpret_cast and the copy_n to the static helper?

[ro]

FWIW, a rebased version of this patch (it needed llvm/Support/Endian.h, among others) fixed the vast majority of mlir failures I'd been
seeing on sparcv9-sun-solaris2.11.

[rriddle]

LGTM after resolving the other comments.

[imaihal]

[mlir] Added copy_n() in static helper

[imaihal]

In D78076#1993928, @ro wrote:

FWIW, a rebased version of this patch (it needed llvm/Support/Endian.h, among others) fixed the vast majority of mlir failures I'd been
seeing on sparcv9-sun-solaris2.11.

Rebased version solved the failures. Thanks!

[imaihal]

In D78076#1991377, @rriddle wrote:

Is there any way you can add a test for this?

Many MLIR test such as "mlir/IR/attributes.mlir" fail on big-endian system, but they are fixed by using this patch.
Should I add additional test for this patch?

[rriddle]

In D78076#2008053, @imaihal wrote:

In D78076#1991377, @rriddle wrote:

Is there any way you can add a test for this?

Many MLIR test such as "mlir/IR/attributes.mlir" fail on big-endian system, but they are fixed by using this patch.
Should I add additional test for this patch?

If the tests already fail and get fixed, +1 on just submitting.

[imaihal]

Pre-merge check fails, but I'm not sure why this happens. Could you tell me how to resolve this? (I'm new about Phabricator and archanist. Excuse me if this is basic question)

I see following error in build log.

Getting dependencies of 260660
Base revision is 9b16ece6ca288d976f61bf81c3986ffcaffe0f72
git reset, git cleanup...
Analyzing D78076
Applying diff 260660 for revision D78076...
Traceback (most recent call last):
  File "/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm-premerge-checks/scripts/phabtalk/apply_patch2.py", line 311, in <module>
    patcher.run()
  File "/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm-premerge-checks/scripts/phabtalk/apply_patch2.py", line 108, in run
    self._apply_diff(self.diff_id, revision_id)
  File "/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/llvm-premerge-checks/scripts/phabtalk/apply_patch2.py", line 224, in _apply_diff
    raise Exception('Applying patch failed:\n{}'.format(proc.stdout + proc.stderr))
Exception: Applying patch failed:
error: patch failed: mlir/lib/IR/Attributes.cpp:524
error: mlir/lib/IR/Attributes.cpp: patch does not apply

[ro]

In D78076#2011725, @imaihal wrote:

Pre-merge check fails, but I'm not sure why this happens. Could you tell me how to resolve this? (I'm new about Phabricator and archanist. Excuse me if this is basic question)

I see following error in build log.

[...]

> Exception: Applying patch failed:
> error: patch failed: mlir/lib/IR/Attributes.cpp:524
> error: mlir/lib/IR/Attributes.cpp: patch does not apply

It's exactly as the error says: your patch doesn't apply cleanly to current master. I noticed the problem myself when trying to re-test
the revised version on Solaris/SPARC:

• For one, the patch is an incremental one on top of your original patch. This won't work: you need to upload a full patch instead.
• Even so, the original patch doesn't apply any longer, but needs to be rebased.

In short: create a complete patch that applies cleanly to current master and upload that. You should be fine then. At least my Solaris testing
after manually doing the same suggests so.

[imaihal]

Rebase

[imaihal]

In D78076#2012022, @ro wrote:

In short: create a complete patch that applies cleanly to current master and upload that. You should be fine then. At least my Solaris testing
after manually doing the same suggests so.

I merged my modification into one commit and rebased it on top of master. Now the tests all passed. Thanks for your help!

[imaihal]

@rriddle Is this patch ready for commit? If so, could you help to commit this? I think I don't have write access to the repository.

[ro]

This is exactly the version I arrived at myself and tested successfully on sparcv9-sun-solaris2.11. Not only does it fix most mlir failures on
that target, but one in particular that would cause ninja check-all to hang until the test is killed manually.

mlir/lib/IR/Attributes.cpp

 #include "mlir/IR/AffineMap.h"
 #include "mlir/IR/Diagnostics.h"
 #include "mlir/IR/Dialect.h"
 #include "mlir/IR/Function.h"
 #include "mlir/IR/IntegerSet.h"
 #include "mlir/IR/Types.h"
 #include "llvm/ADT/Sequence.h"
 #include "llvm/ADT/Twine.h"
+#include "llvm/Support/Endian.h"
 
 using namespace mlir;
 using namespace mlir::detail;
 
 //===----------------------------------------------------------------------===//
 // AttributeStorage
 //===----------------------------------------------------------------------===//
 
else
     rawData[bitPos / CHAR_BIT] &= ~(1 << (bitPos % CHAR_BIT));
 }
 
 /// Return the value of the specified bit.
 static bool getBit(const char *rawData, size_t bitPos) {
   return (rawData[bitPos / CHAR_BIT] & (1 << (bitPos % CHAR_BIT))) != 0;
 }
 
+/// Get start position of actual data in `value`. Actual data is
+/// stored in last `bitWidth`/CHAR_BIT bytes in big endian.
+static char *getAPIntDataPos(APInt &value, size_t bitWidth) {
+  char *dataPos =
+      const_cast<char *>(reinterpret_cast<const char *>(value.getRawData()));
+  if (llvm::support::endian::system_endianness() ==
+      llvm::support::endianness::big)
+    dataPos = dataPos + 8 - llvm::divideCeil(bitWidth, CHAR_BIT);
+  return dataPos;
+}
+
+/// Read APInt `value` from appropriate position.
+static void readAPInt(APInt &value, size_t bitWidth, char *outData) {
+  char *dataPos = getAPIntDataPos(value, bitWidth);
+  std::copy_n(dataPos, llvm::divideCeil(bitWidth, CHAR_BIT), outData);
+}
+
+/// Write `inData` to appropriate position of APInt `value`.
+static void writeAPInt(const char *inData, size_t bitWidth, APInt &value) {
+  char *dataPos = getAPIntDataPos(value, bitWidth);
+  std::copy_n(inData, llvm::divideCeil(bitWidth, CHAR_BIT), dataPos);
+}
+
 /// Writes value to the bit position `bitPos` in array `rawData`.
 static void writeBits(char *rawData, size_t bitPos, APInt value) {
   size_t bitWidth = value.getBitWidth();
 
   // If the bitwidth is 1 we just toggle the specific bit.
   if (bitWidth == 1)
     return setBit(rawData, bitPos, value.isOneValue());
 
   // Otherwise, the bit position is guaranteed to be byte aligned.
   assert((bitPos % CHAR_BIT) == 0 && "expected bitPos to be 8-bit aligned");
-  std::copy_n(reinterpret_cast<const char *>(value.getRawData()),
-              llvm::divideCeil(bitWidth, CHAR_BIT),
-              rawData + (bitPos / CHAR_BIT));
+  readAPInt(value, bitWidth, rawData + (bitPos / CHAR_BIT));
 }
 
 /// Reads the next `bitWidth` bits from the bit position `bitPos` in array
 /// `rawData`.
 static APInt readBits(const char *rawData, size_t bitPos, size_t bitWidth) {
   // Handle a boolean bit position.
   if (bitWidth == 1)
     return APInt(1, getBit(rawData, bitPos) ? 1 : 0);
 
   // Otherwise, the bit position must be 8-bit aligned.
   assert((bitPos % CHAR_BIT) == 0 && "expected bitPos to be 8-bit aligned");
   APInt result(bitWidth, 0);
-  std::copy_n(
-      rawData + (bitPos / CHAR_BIT), llvm::divideCeil(bitWidth, CHAR_BIT),
-      const_cast<char *>(reinterpret_cast<const char *>(result.getRawData())));
+  writeAPInt(rawData + (bitPos / CHAR_BIT), bitWidth, result);
   return result;
 }
 
 /// Returns if 'values' corresponds to a splat, i.e. one element, or has the
 /// same element count as 'type'.
 template <typename Values>
 static bool hasSameElementsOrSplat(ShapedType type, const Values &values) {
   return (values.size() == 1) ||