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

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: mlir/lib/Support/EndianUtilities.cpp:44
+
+void mlir::convEndianBE(ArrayRef<char> inRawData, ArrayRef<char> outRawData,
+                        ShapedType type) {
----------------
I expect a MutableArrayRef here?




================
Comment at: mlir/lib/Support/EndianUtilities.cpp:52
+  }
+  size_t storageBitWidth = getDenseElementStorageWidth(elementType);
+  if (storageBitWidth == 16) {
----------------
Can you `assert(numElements*storageBitWidth == inRawData.size())` and `inRawData.size() <= outRawData.size()` ?


================
Comment at: mlir/lib/Support/EndianUtilities.cpp:54
+  if (storageBitWidth == 16) {
+    ulittle16_t *inRawDataPos = const_cast<ulittle16_t *>(
+        reinterpret_cast<const ulittle16_t *>(inRawData.begin()));
----------------
It does not work with `const ulittle16_t *` ?

If so, can you extract the const_cast outside of the sequence of `if` (and make it a switch?):
```
char *rawDataBegin = const_cast<char *>(inRawData.begin());
switch (storageBitWidth) {
  case 16: {
    ulittle16_t *inRawDataPos =  reinterpret_cast<ulittle16_t *>(rawDataBegin);
    uint16_t *outDataPos  = reinterpret_cast<uint16_t *>(outRawData.begin());
    for (unsigned i = 0, e = numElements; i < e; ++i)
      std::copy_n(inRawDataPos + i, 1, outDataPos + i);
  }
  ...
```


================
Comment at: mlir/lib/Support/EndianUtilities.cpp:59
+    for (unsigned i = 0, e = numElements; i < e; ++i)
+      std::copy_n(inRawDataPos + i, 1, outDataPos + i);
+  } else if (storageBitWidth == 32) {
----------------
It isn't clear to me right now why you need a loop instead of using the second argument of std::copy_n?

```
std::copy_n(inRawDataPos, numElements, outDataPos);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80695