[PATCH] D78600: DenseStringElementsAttr added to default attribute types

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Thanks for adding this! I think this can prove to be extremely useful for dense string storage. Can you beef up the description a bit to include a bit more of the internal design? i.e. what strategy you are using to make it "dense".



================
Comment at: mlir/lib/IR/AttributeDetail.h:622
+    ArrayRef<StringRef> copy, data = key.data;
+    if (!data.empty()) {
+      int numEntries = key.isSplat ? 1 : data.size();
----------------
nit: This block is a bit long, can you switch to early exit here?


================
Comment at: mlir/lib/IR/AttributeDetail.h:636
+      // Setup the ArrayRef
+      auto mutable_copy = MutableArrayRef<StringRef>(
+          reinterpret_cast<StringRef *>(rawData), numEntries);
----------------
Please use camelCase for variable names.


================
Comment at: mlir/lib/Parser/Parser.cpp:2302
+
+  if (parseToken(Token::less, "expected '<' after 'dense'")) {
     return nullptr;
----------------
nit: Drop trivial braces.


================
Comment at: mlir/test/lib/Dialect/Test/TestDialect.cpp:169
 
+Type TestDialect::parseType(DialectAsmParser &parser) const {
+  Location loc = parser.getEncodedSourceLoc(parser.getNameLoc());
----------------
Can you replace all of the custom type stuff here with just an opaque type? Opaque types don't need to be registered, and seem like they could serve the same purpose without any additional code. I think you could just add `allowUnknownTypes` to the constructor of TestDialect, remove all of the other C++ for Test types, and the tests would still work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78600