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

[River Riddle via Phabricator via llvm-commits]

rriddle accepted this revision.
rriddle added a comment.
This revision is now accepted and ready to land.

LGTM after resolving the remaining comments. Thanks for pushing on this!



================
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
----------------
imaihal wrote:
> rriddle wrote:
> > Should we even support calling this if the machine is already LE?
> We don't have to call this function in LE machines, but we can call this even in LE machines. 
> LE machine:
> `inRawData` LE -> `outRawData` LE
> BE machine:
>  `inRawData` LE -> `outRawData` BE
> 
> As you wrote before, there are redundant copy when we use this function in LE machine. To avoid this redundant copy, I think we need to check machine endianness as in this patch. 
Yeah, that's why I'd be okay with asserting inside of these functions. It could prevent some accidental usages of these functions when they shouldn't be used. Dense data gets extremely large.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:1517
+      MutableArrayRef<char> convRawData(outDataVec);
+      DenseIntOrFPElementsAttr::convertEndianOfArrayRefForBEmachine(
+          rawData, convRawData, type);
----------------
imaihal wrote:
> rriddle wrote:
> > I may be confused right now, but doesn't `convertEndianOfArrayRefForBEmachine` just convert from little to big and not big to little?
> This may be confusing, but this also converts big to little on BE machine.
> 
> `copy_n` copies `inRawData`(`ulittle`) to `outRawData`(`uint`).  This copy assumes `inRawData` is LE format. So, this copy_n always converts endianness on BE machine even in actual `inRawData` is BE format.
> 
> Normally this is used when `inRawData` is LE format, but I reused to convert BE to LE.
Okay, that kind of makes sense now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80695