[PATCH] D78600: DenseStringElementsAttr added to default attribute types

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

It seems like right now, the storage strategy is:

<StringRef * N><StringData>

Have you considered other strategies?

1. You could unique the strings on construction and build a string table instead of storing each string individually.

2. Instead of storing StringRef for each element, you could store an index into the string data. If you don't unique the strings, the size of the string is just the `nextOffset - current`. If you do unique, you can store the length as a string table entry.

Can you share some examples of the types of data that you can expect to be stored here? and any characteristics we might want to optimize around.



================
Comment at: mlir/include/mlir/IR/Attributes.h:145
   /// Elements Attributes.
   DenseElements,
+  DenseStringElements,
----------------
Can we rename this to DenseIntOrFPElements?


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


================
Comment at: mlir/include/mlir/IR/Attributes.h:995
+
+class DenseStringElementsAttr
+    : public Attribute::AttrBase<DenseStringElementsAttr, DenseElementsAttr,
----------------
Can you add class comments to these?


================
Comment at: mlir/include/mlir/IR/Attributes.h:1008
+  /// Return the held element values as a range of StringRefs.
+  ArrayRef<StringRef> getStringValues() const;
+
----------------
I would have expected this to return a range of ElementIterator<StringRef>. If the data is a splat, we will need to repeat the value N times.

Also, can we sink this into DenseElementsAttr?


================
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()))
----------------
Can you add this to DenseStringElementsAttr as well? We want to avoid pretty printing huge tensors.


================
Comment at: mlir/lib/IR/AsmPrinter.cpp:1557
+  if (attr.isSplat()) {
+    // TODO(suderman): Add a print for a single value.
+    printDenseStringElement(attr, os, 0);
----------------
nit: Drop the username from the TODO.


================
Comment at: mlir/lib/IR/AttributeDetail.h:589
+    // directly the data buffer.
+    if (isKnownSplat) {
+      return KeyTy(ty, data, llvm::hash_value(data), isKnownSplat);
----------------
nit: remove trivial braces.


================
Comment at: mlir/lib/IR/AttributeDetail.h:631
+    // contents.
+    size_t dataSize = sizeof(ArrayRef<StringRef>) * numEntries;
+    for (int i = 0; i < numEntries; i++) {
----------------
Did you mean `sizeof(StringRef) * numEntries`?


================
Comment at: mlir/lib/IR/AttributeDetail.h:632
+    size_t dataSize = sizeof(ArrayRef<StringRef>) * numEntries;
+    for (int i = 0; i < numEntries; i++) {
+      dataSize += data[i].size();
----------------
nit: Drop trivial braces.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78600