[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:1145
+  /// `inRawData` LE -> `outRawData` BE, `inRawData` BE -> `outRawData` LE
+  static void convEndianBE(ArrayRef<char> inRawData,
+                           MutableArrayRef<char> outRawData, ShapedType type);
----------------
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.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:1509
+    ArrayRef<char> inRawData = attr.getRawData();
+    // Convert endianess in big-endian(BE) machines. `inRawData` is BE in BE
+    // machines. It is converted here to print in LE format. Not converted
----------------
If this machine is already little-endian, can we remove the redundant copy?


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:1512-1514
+    SmallVector<char, 128> outDataVec;
+    MutableArrayRef<char> rawData =
+        llvm::makeMutableArrayRef((char *)outDataVec.data(), inRawData.size());
----------------
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.


================
Comment at: mlir/lib/IR/Attributes.cpp:24
 using namespace mlir::detail;
+using llvm::support::ulittle16_t;
+using llvm::support::ulittle32_t;
----------------
Can you move these inside of convEndianBE?


================
Comment at: mlir/lib/Parser/AttributeParser.cpp:705
 
+  // Convert endianess in big-endian(BE) machines. `inRawData` is
+  // little-endian(LE) because HEX in raw data of dense element attribute
----------------
If this machine is already little-endian, can we remove the redundant copy?


================
Comment at: mlir/lib/Parser/AttributeParser.cpp:710
+  SmallVector<char, 128> outDataVec;
+  MutableArrayRef<char> rawData =
+      llvm::makeMutableArrayRef((char *)outDataVec.data(), inRawData.size());
----------------
Same comments here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80695