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

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.
Herald added a subscriber: msifontes.
Herald added a project: MLIR.

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



================
Comment at: mlir/lib/IR/Attributes.cpp:549
 /// stored in last `bitWidth`/CHAR_BIT bytes in big endian.
-static char *getAPIntDataPos(APInt &value, size_t bitWidth) {
+static char *getAPIntDataPosBE(APInt &value, size_t bitWidth, size_t wordNum) {
   char *dataPos =
----------------
nit: I would spell out big endian in the function name, or at least put it in the comment above.


================
Comment at: mlir/lib/IR/Attributes.cpp:556
+            llvm::divideCeil(bitWidth, CHAR_BIT);
+
   return dataPos;
----------------
nit: Remove this newline.


================
Comment at: mlir/lib/IR/Attributes.cpp:566
+      llvm::support::endianness::big) {
+    unsigned numFilledWords = bitWidth / APInt::APINT_BITS_PER_WORD;
+    unsigned resBits = bitWidth - numFilledWords * APInt::APINT_BITS_PER_WORD;
----------------
This looks identical to below, can you extract into a helper?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80272