[mlir] Support big-endian systems in DenseElementsAttr (multiple word)

[imaihal]

Summary

D78076 supports big endian in DenseElementsAttr, but does not work when
APInt has multiple words(the number of bits > 64). This patch updates
D78076 to support it.
This patch removed the fix in D78076 and re-implemented to support multiple words.

[imaihal]

Fixed variable name to meet camelBack style

[rriddle]

Sorry for the delay, I've been OOO. Looks good, just a couple of comments.

mlir/lib/IR/Attributes.cpp

569–570

nit: I would spell out big endian in the function name, or at least put it in the comment above.

573

nit: Remove this newline.

573

This looks identical to below, can you extract into a helper?

[efriedma]

In general, I'd like to strongly discourage directly checking endian::system_endianness(), in favor of using the endian-sensitive read/write helpers from llvm/Support/Endian.h. For example, you can copy_n from an endian::ulittle64_t* to a uint64_t*. Using a distinct codepath for big-endian means more code, and that code gets less testing.

[rriddle]

In D80272#2077637, @efriedma wrote:

In general, I'd like to strongly discourage directly checking endian::system_endianness(), in favor of using the endian-sensitive read/write helpers from llvm/Support/Endian.h. For example, you can copy_n from an endian::ulittle64_t* to a uint64_t*. Using a distinct codepath for big-endian means more code, and that code gets less testing.

Thanks for the comments Eli! Didn't know about those. Will recommend the same in the future.

[imaihal]

Reflect reviewer's comments.

[imaihal]

mlir/lib/IR/Attributes.cpp

573

Created getAPIntCopyParam() to calculation parameters for copying.

[imaihal]

In D80272#2077654, @rriddle wrote:

In D80272#2077637, @efriedma wrote:

In general, I'd like to strongly discourage directly checking endian::system_endianness(), in favor of using the endian-sensitive read/write helpers from llvm/Support/Endian.h. For example, you can copy_n from an endian::ulittle64_t* to a uint64_t*. Using a distinct codepath for big-endian means more code, and that code gets less testing.

Thanks for the comments Eli! Didn't know about those. Will recommend the same in the future.

I didn't know that. I will check it and create another patch in the future.

[rriddle]

Can you try Eli's suggestion in this patch?

[imaihal]

Use ulittle for eidian conversion.

[imaihal]

Updated comments and inserted assert.

[imaihal]

In D80272#2090239, @rriddle wrote:

Can you try Eli's suggestion in this patch?

@rriddle Sorry for late response. He suggested we should not check endianenss, This means we should use the same code in BE and LE machines.
However, I think redundant copy are required in LE to use the same code. So, current patch checks the endianness to avoid it.
Do you have any thought?

[imaihal]

Hi, @rriddle, do you have any comments on this patch?

[rriddle]

Apologies for the delay. LGTM as seems we are reusing the same methods we added in the patch for parsing.

[imaihal]

@rriddle Thanks for reviewing many times! I don't have commit access. Could you commit this patch if you don't have any other comments?

mlir/lib/IR/Attributes.cpp

static void setBit(char *rawData, size_t bitPos, bool value) {
   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;
-}
+/// Copy actual `numBytes` data from `value` (APInt) to char array(`result`) for
+/// BE format.
+static void copyAPIntToArrayForBEmachine(APInt value, size_t numBytes,
+                                         char *result) {
+  assert(llvm::support::endian::system_endianness() == // NOLINT
+         llvm::support::endianness::big);              // NOLINT
+  assert(value.getNumWords() * APInt::APINT_WORD_SIZE >= numBytes);
 
-/// 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);
-}
+  // Copy the words filled with data.
+  // For example, when `value` has 2 words, the first word is filled with data.
+  // `value` (10 bytes, BE):|abcdefgh|------ij| ==> `result` (BE):|abcdefgh|--|
+  size_t numFilledWords = (value.getNumWords() - 1) * APInt::APINT_WORD_SIZE;
+  std::copy_n(reinterpret_cast<const char *>(value.getRawData()),
+              numFilledWords, result);
+  // Convert last word of APInt to LE format and store it in char
+  // array(`valueLE`).
+  // ex. last word of `value` (BE): |------ij|  ==> `valueLE` (LE): |ji------|
+  size_t lastWordPos = numFilledWords;
+  SmallVector<char, 8> valueLE(APInt::APINT_WORD_SIZE);
+  DenseIntOrFPElementsAttr::convertEndianOfCharForBEmachine(
+      reinterpret_cast<const char *>(value.getRawData()) + lastWordPos,
+      valueLE.begin(), APInt::APINT_BITS_PER_WORD, 1);
+  // Extract actual APInt data from `valueLE`, convert endianness to BE format,
+  // and store it in `result`.
+  // ex. `valueLE` (LE): |ji------|  ==> `result` (BE): |abcdefgh|ij|
+  DenseIntOrFPElementsAttr::convertEndianOfCharForBEmachine(
+      valueLE.begin(), result + lastWordPos,
+      (numBytes - lastWordPos) * CHAR_BIT, 1);
+}
+
+/// Copy `numBytes` data from `inArray`(char array) to `result`(APINT) for BE
+/// format.
+static void copyArrayToAPIntForBEmachine(const char *inArray, size_t numBytes,
+                                         APInt &result) {
+  assert(llvm::support::endian::system_endianness() == // NOLINT
+         llvm::support::endianness::big);              // NOLINT
+  assert(result.getNumWords() * APInt::APINT_WORD_SIZE >= numBytes);
 
-/// 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);
+  // Copy the data that fills the word of `result` from `inArray`.
+  // For example, when `result` has 2 words, the first word will be filled with
+  // data. So, the first 8 bytes are copied from `inArray` here.
+  // `inArray` (10 bytes, BE): |abcdefgh|ij|
+  //                     ==> `result` (2 words, BE): |abcdefgh|--------|
+  size_t numFilledWords = (result.getNumWords() - 1) * APInt::APINT_WORD_SIZE;
+  std::copy_n(
+      inArray, numFilledWords,
+      const_cast<char *>(reinterpret_cast<const char *>(result.getRawData())));
+
+  // Convert array data which will be last word of `result` to LE format, and
+  // store it in char array(`inArrayLE`).
+  // ex. `inArray` (last two bytes, BE): |ij|  ==> `inArrayLE` (LE): |ji------|
+  size_t lastWordPos = numFilledWords;
+  SmallVector<char, 8> inArrayLE(APInt::APINT_WORD_SIZE);
+  DenseIntOrFPElementsAttr::convertEndianOfCharForBEmachine(
+      inArray + lastWordPos, inArrayLE.begin(),
+      (numBytes - lastWordPos) * CHAR_BIT, 1);
+
+  // Convert `inArrayLE` to BE format, and store it in last word of `result`.
+  // ex. `inArrayLE` (LE): |ji------|  ==> `result` (BE): |abcdefgh|------ij|
+  DenseIntOrFPElementsAttr::convertEndianOfCharForBEmachine(
+      inArrayLE.begin(),
+      const_cast<char *>(reinterpret_cast<const char *>(result.getRawData())) +
+          lastWordPos,
+      APInt::APINT_BITS_PER_WORD, 1);
 }
 
 /// 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");
-  readAPInt(value, bitWidth, rawData + (bitPos / CHAR_BIT));
+  if (llvm::support::endian::system_endianness() ==
+      llvm::support::endianness::big) {
+    // Copy from `value` to `rawData + (bitPos / CHAR_BIT)`.
+    // Copying the first `llvm::divideCeil(bitWidth, CHAR_BIT)` bytes doesn't
+    // work correctly in BE format.
+    // ex. `value` (2 words including 10 bytes)
+    // ==> BE: |abcdefgh|------ij|,  LE: |hgfedcba|ji------|
+    copyAPIntToArrayForBEmachine(value, llvm::divideCeil(bitWidth, CHAR_BIT),
+                                 rawData + (bitPos / CHAR_BIT));
+  } else {
+    std::copy_n(reinterpret_cast<const char *>(value.getRawData()),
+                llvm::divideCeil(bitWidth, CHAR_BIT),
+                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);
-  writeAPInt(rawData + (bitPos / CHAR_BIT), bitWidth, result);
+  if (llvm::support::endian::system_endianness() ==
+      llvm::support::endianness::big) {
+    // Copy from `rawData + (bitPos / CHAR_BIT)` to `result`.
+    // Copying the first `llvm::divideCeil(bitWidth, CHAR_BIT)` bytes doesn't
+    // work correctly in BE format.
+    // ex. `result` (2 words including 10 bytes)
+    // ==> BE: |abcdefgh|------ij|,  LE: |hgfedcba|ji------| This function
+    copyArrayToAPIntForBEmachine(rawData + (bitPos / CHAR_BIT),
+                                 llvm::divideCeil(bitWidth, CHAR_BIT), result);
+  } else {
+    std::copy_n(rawData + (bitPos / CHAR_BIT),
+                llvm::divideCeil(bitWidth, CHAR_BIT),
+                const_cast<char *>(
+                    reinterpret_cast<const char *>(result.getRawData())));
+  }
   return result;
 }
 
 /// Returns true 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) ||
const ulittle64_t *inRawDataPos =
         reinterpret_cast<const ulittle64_t *>(inRawData);
     uint64_t *outDataPos = reinterpret_cast<uint64_t *>(outRawData);
     std::copy_n(inRawDataPos, numElements, outDataPos);
     break;
   }
   default: {
     size_t nBytes = elementBitWidth / CHAR_BIT;
     for (size_t i = 0; i < nBytes; i++)
-      std::copy_n(inRawData + (nBytes - 1 - i), numElements, outRawData + i);
+      std::copy_n(inRawData + (nBytes - 1 - i), 1, outRawData + i);
     break;
   }
   }
 }
 
 void DenseIntOrFPElementsAttr::convertEndianOfArrayRefForBEmachine(
     ArrayRef<char> inRawData, MutableArrayRef<char> outRawData,
     ShapedType type) {