[PATCH] D72977: [mlir][ods] Fix StringRef initialization in builders

[Lei Zhang via Phabricator via llvm-commits]

antiagainst created this revision.
antiagainst added reviewers: rriddle, jpienaar.
Herald added subscribers: llvm-commits, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob, shauheen, burmako, mehdi_amini.
Herald added a reviewer: nicolasvasilache.
Herald added a project: LLVM.
antiagainst updated this revision to Diff 238950.
antiagainst added a comment.

Revise comment


For the generated builder taking in unwrapped attribute values,
if the argument is a string, we should avoid wrapping it in quotes;
otherwise we are always setting the string attribute to contain
the string argument's name. The quotes come from StrinAttr's
`constBuilderCall`, which is reasonable for string literals, but
not function arguments containing strings.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72977

Files:
  mlir/include/mlir/TableGen/Attribute.h
  mlir/test/mlir-tblgen/op-attribute.td
  mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp


Index: mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
===================================================================
--- mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -12,6 +12,7 @@
 //===----------------------------------------------------------------------===//
 
 #include "mlir/Support/STLExtras.h"
+#include "mlir/Support/StringExtras.h"
 #include "mlir/TableGen/Format.h"
 #include "mlir/TableGen/GenInfo.h"
 #include "mlir/TableGen/OpClass.h"
@@ -91,6 +92,29 @@
 // Utility structs and functions
 //===----------------------------------------------------------------------===//
 
+/// Splits the given `input` string using the given `delimiter` and writes the
+/// segments into `outputs`. `delimiter` will be treated as a whole for
+/// splitting. If a segment is empty, it will be omitted.
+static void splitString(StringRef input, StringRef delimiter,
+                        llvm::SmallVectorImpl<StringRef> &outputs) {
+  outputs.clear();
+
+  // Location in input to start scanning for the next delimiter.
+  size_t scanLoc = 0;
+  // Location in input for the next delimiter.
+  size_t splitLoc = StringRef::npos;
+
+  while ((splitLoc = input.find(delimiter, scanLoc)) != StringRef::npos) {
+    if (scanLoc < splitLoc)
+      outputs.push_back(input.substr(scanLoc, splitLoc - scanLoc));
+    scanLoc = splitLoc + delimiter.size();
+  }
+
+  // Push the last fragment if not empty.
+  if (scanLoc < input.size())
+    outputs.push_back(input.substr(scanLoc, input.size() - scanLoc));
+}
+
 // Returns whether the record has a value of the given name that can be returned
 // via getValueAsString.
 static inline bool hasStringAttribute(const Record &record,
@@ -968,8 +992,20 @@
         // instance.
         FmtContext fctx;
         fctx.withBuilder("(*odsBuilder)");
-        std::string value =
-            tgfmt(attr.getConstBuilderTemplate(), &fctx, namedAttr.name);
+
+        std::string builderTemplate = attr.getConstBuilderTemplate();
+
+        // For StringAttr, its constant builder call will wrap the input in
+        // quotes, which is correct for normal string literals, but incorrect
+        // here given we use function arguments. So we need to strip the
+        // wrapping quotes.
+        if (StringRef(builderTemplate).contains("\"$0\"")) {
+          SmallVector<StringRef, 2> segments;
+          splitString(builderTemplate, "\"$0\"", segments);
+          builderTemplate = llvm::join(segments, "$0");
+        }
+
+        std::string value = tgfmt(builderTemplate, &fctx, namedAttr.name);
         body << formatv("  {0}.addAttribute(\"{1}\", {2});\n", builderOpState,
                         namedAttr.name, value);
       } else {
Index: mlir/test/mlir-tblgen/op-attribute.td
===================================================================
--- mlir/test/mlir-tblgen/op-attribute.td
+++ mlir/test/mlir-tblgen/op-attribute.td
@@ -186,6 +186,11 @@
 // DECL-LABEL: DOp declarations
 // DECL: static void build({{.*}}, APInt i32_attr, APFloat f64_attr, StringRef str_attr, bool bool_attr, ::SomeI32Enum enum_attr, APInt dv_i32_attr, APFloat dv_f64_attr, StringRef dv_str_attr = "abc", bool dv_bool_attr = true, ::SomeI32Enum dv_enum_attr = ::SomeI32Enum::case5)
 
+// DEF-LABEL: DOp definitions
+// DEF: odsState.addAttribute("str_attr", (*odsBuilder).getStringAttr(str_attr));
+// DEF: odsState.addAttribute("dv_str_attr", (*odsBuilder).getStringAttr(dv_str_attr));
+
+
 // Test that only default valued attributes at the end of the arguments
 // list get default values in the builder signature
 // ---
Index: mlir/include/mlir/TableGen/Attribute.h
===================================================================
--- mlir/include/mlir/TableGen/Attribute.h
+++ mlir/include/mlir/TableGen/Attribute.h
@@ -64,8 +64,8 @@
 
   // Returns the template that can be used to produce an instance of the
   // attribute.
-  // Syntax: {0} should be replaced with a builder, {1} should be replaced with
-  // the constant value.
+  // Syntax: `$builder` should be replaced with a builder, `$0` should be
+  // replaced with the constant value.
   StringRef getConstBuilderTemplate() const;
 
   // Returns the base-level attribute that this attribute constraint is


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72977.238950.patch
Type: text/x-patch
Size: 4265 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200118/88fd3c92/attachment.bin>