[PATCH] D78076: [mlir] Support big endian in DenseElementsAttr

[Haruki Imai via Phabricator via llvm-commits]

imaihal created this revision.
Herald added subscribers: llvm-commits, frgossen, grosul1, Joonsoo, liufengdb, lucyrfox, mgester, arpith-jacob, nicolasvasilache, antiagainst, shauheen, burmako, jpienaar, rriddle, mehdi_amini.
Herald added a reviewer: rriddle.
Herald added a project: LLVM.
rriddle added a comment.

Thanks for the patch!



================
Comment at: mlir/lib/IR/Attributes.cpp:522
   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),
+  const char *cptr = reinterpret_cast<const char *>(value.getRawData());
+  if (llvm::support::endian::system_endianness() ==
----------------
Can you extract this to a static helper above this function and add comments as to what is going on?


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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D78076

Files:
  mlir/lib/IR/Attributes.cpp


Index: mlir/lib/IR/Attributes.cpp
===================================================================
--- mlir/lib/IR/Attributes.cpp
+++ mlir/lib/IR/Attributes.cpp
@@ -16,6 +16,7 @@
 #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;
@@ -518,8 +519,11 @@
 
   // 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),
+  const char *cptr = reinterpret_cast<const char *>(value.getRawData());
+  if (llvm::support::endian::system_endianness() ==
+      llvm::support::endianness::big)
+    cptr = cptr + 8 - llvm::divideCeil(bitWidth, CHAR_BIT);
+  std::copy_n(cptr, llvm::divideCeil(bitWidth, CHAR_BIT),
               rawData + (bitPos / CHAR_BIT));
 }
 
@@ -533,9 +537,13 @@
   // 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())));
+  char *cptr =
+      const_cast<char *>(reinterpret_cast<const char *>(result.getRawData()));
+  if (llvm::support::endian::system_endianness() ==
+      llvm::support::endianness::big)
+    cptr = cptr + 8 - llvm::divideCeil(bitWidth, CHAR_BIT);
+  std::copy_n(rawData + (bitPos / CHAR_BIT),
+              llvm::divideCeil(bitWidth, CHAR_BIT), cptr);
   return result;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D78076.257185.patch
Type: text/x-patch
Size: 1725 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200414/f3c0e93f/attachment.bin>