[PATCH] D78600: DenseStringElementsAttr added to default attribute types

[Rob Suderman via Phabricator via llvm-commits]

rsuderman marked an inline comment as done.
rsuderman added a comment.

Updated for the most part. I am not sure we can handle a dense serialization format like floats / strings due to the variable length of our data.



================
Comment at: mlir/include/mlir/IR/Attributes.h:942
   }
   FloatElementIterator float_value_begin() const;
   FloatElementIterator float_value_end() const;
----------------
rriddle wrote:
> Can you add the proper equivalents `getValues<StringRef>` accessors? It also allows for getValue/getSplatValue/etc. to "just work".
Updated to include both getValues and renamed getStringValues to getStringRefs.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:1508
   if (allowHex && shouldPrintElementsAttrWithHex(numElements)) {
     ArrayRef<char> rawData = attr.getRawData();
     os << '"' << "0x" << llvm::toHex(StringRef(rawData.data(), rawData.size()))
----------------
rriddle wrote:
> Can you add this to DenseStringElementsAttr as well? We want to avoid pretty printing huge tensors.
I am not certain the hex printer is going to be usable for printing multiple string attributes. Any binary representation of strings depends on encoding the number of elements, followed by the full binary data for each part. If you have a recommended serialization format that we can use for multiple strings, I am happy to look into it, however the current hex format won't be able to express hex strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78600