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

[River Riddle via Phabricator via llvm-commits]

rriddle added inline comments.


================
Comment at: mlir/include/mlir/IR/Attributes.h:1143
 
+  /// Convert endianess of input ArrayRef for big-endian(BE) machines. All of
+  /// the elements of `inRawData` has `type`. If `inRawData` is little endian
----------------
Should we even support calling this if the machine is already LE?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:1517
+      MutableArrayRef<char> convRawData(outDataVec);
+      DenseIntOrFPElementsAttr::convertEndianOfArrayRefForBEmachine(
+          rawData, convRawData, type);
----------------
I may be confused right now, but doesn't `convertEndianOfArrayRefForBEmachine` just convert from little to big and not big to little?


================
Comment at: mlir/lib/IR/Attributes.cpp:1123
+    const char *inRawData, char *outRawData, size_t elementBitWidth,
+    size_t numElements) {
+  using llvm::support::ulittle16_t;
----------------
Add an assert that the current machine is big endian here and in the function below?


================
Comment at: mlir/lib/IR/Attributes.cpp:1154
+    // Only when endianess is BE. Not necessary for LE.
+    if (system_endianness() == big) {
+      size_t nBytes = elementBitWidth / CHAR_BIT;
----------------
Is this check necessary anymore?


================
Comment at: mlir/lib/IR/Attributes.cpp:1156
+      size_t nBytes = elementBitWidth / CHAR_BIT;
+      for (int i = 0; i < (int)nBytes; i++)
+        std::copy_n(inRawData + (nBytes - 1 - i), numElements, outRawData + i);
----------------
nit: Just use `size_t` for `i` to remove the cast on `nBytes`. Or switch both to `ssize_t`.


================
Comment at: mlir/lib/IR/Attributes.cpp:1162
+  }
+  return;
+}
----------------
Drop this empty return.


================
Comment at: mlir/lib/IR/Attributes.cpp:1177
+         inRawData.size() <= outRawData.size());
+  DenseIntOrFPElementsAttr::convertEndianOfCharForBEmachine(
+      inRawData.begin(), outRawData.begin(), elementBitWidth, numElements);
----------------
Can you drop the `DenseIntOrFPElementsAttr::` here?


================
Comment at: mlir/lib/Parser/AttributeParser.cpp:686
                                                   ShapedType type) {
+  using llvm::support::endian::system_endianness;
+  using llvm::support::endianness::big;
----------------
Can you just use these inline and drop the `using`? I don't think it saves much.


================
Comment at: mlir/lib/Parser/AttributeParser.cpp:720
+                                               detectedSplat);
+  } else {
+    return DenseElementsAttr::getFromRawBuffer(type, rawData, detectedSplat);
----------------
nit: Drop the else after return here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80695/new/

https://reviews.llvm.org/D80695