[mlir][ods] Fix StringRef initialization in builders

[antiagainst]

Summary

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.

[antiagainst]

Revise comment

[merge_guards_bot]

Unit tests: pass. 61989 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[merge_guards_bot]

Unit tests: pass. 61989 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

98

Why not just add a replaceAllSubStrs using std::string instead? Generally only takes ~5 lines of code.

[antiagainst]

Address comment

[antiagainst]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

98

Sure, that works too. Changed.

[merge_guards_bot]

Unit tests: pass. 61989 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[rriddle]

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

990

Remove trivial brace.

mlir/include/mlir/TableGen/Attribute.h

public:
   // The call will contain a `{0}` which will be expanded to this attribute.
   StringRef getConvertFromStorageCall() const;
 
   // Returns true if this attribute can be built from a constant value.
   bool isConstBuildable() const;
 
   // 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
   // built upon.
   Attribute getBaseAttr() const;
 
   // Returns whether this attribute has a default value.
   bool hasDefaultValue() const;

mlir/test/mlir-tblgen/op-attribute.td

let arguments = (ins
     DefaultValuedAttr<BoolAttr, "true">:$dv_bool_attr,
     DefaultValuedAttr<SomeI32Enum, "::SomeI32Enum::case5">:$dv_enum_attr
   );
 }
 
 // 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
 // ---
 
 def EOp : NS_Op<"e_op", []> {
   let arguments = (ins
     I32Attr:$i32_attr,
     DefaultValuedAttr<I32Attr, "42">:$dv_i32_attr,

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

 //===- OpDefinitionsGen.cpp - MLIR op definitions generator ---------------===//
 //
 // Part of the MLIR Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // OpDefinitionsGen uses the description of operations to generate C++
 // definitions for ops.
 //
 //===----------------------------------------------------------------------===//
 
 #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"
 #include "mlir/TableGen/OpInterfaces.h"
 #include "mlir/TableGen/OpTrait.h"
 #include "mlir/TableGen/Operator.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Signals.h"

 //===----------------------------------------------------------------------===//
 
 )";
 
 //===----------------------------------------------------------------------===//
 // Utility structs and functions
 //===----------------------------------------------------------------------===//
 
+// Replaces all occurrences of `match` in `str` with `substitute`.
+static std::string replaceAllSubstrs(std::string str, const std::string &match,
+                                     const std::string &substitute) {
+  std::string::size_type scanLoc = 0, matchLoc = std::string::npos;
+  while ((matchLoc = str.find(match, scanLoc)) != std::string::npos) {
+    str = str.replace(matchLoc, match.size(), substitute);
+    scanLoc = matchLoc + substitute.size();
+  }
+  return str;
+}
+
 // Returns whether the record has a value of the given name that can be returned
 // via getValueAsString.
 static inline bool hasStringAttribute(const Record &record,
                                       StringRef fieldName) {
   auto valueInit = record.getValueInit(fieldName);
   return isa<CodeInit>(valueInit) || isa<StringInit>(valueInit);
 }
 
if (!attr.isDerivedAttr()) {
       if (emitNotNullCheck) {
         body << formatv("  if ({0}) ", namedAttr.name) << "{\n";
       }
       if (isRawValueAttr && canUseUnwrappedRawValue(attr)) {
         // If this is a raw value, then we need to wrap it in an Attribute
         // 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\""))
+          builderTemplate = replaceAllSubstrs(builderTemplate, "\"$0\"", "$0");
+
+        std::string value = tgfmt(builderTemplate, &fctx, namedAttr.name);
         body << formatv("  {0}.addAttribute(\"{1}\", {2});\n", builderOpState,
                         namedAttr.name, value);
       } else {
         body << formatv("  {0}.addAttribute(\"{1}\", {1});\n", builderOpState,
                         namedAttr.name);
       }
       if (emitNotNullCheck) {
         body << "  }\n";