[MLIR][ODS] default-valued strings should be in quotes

[Mogball]

Summary

DefaultValuedAttr<StrAttr, ""> and ConstantAttr<StrAttr, "">
result in bugs in which TableGen will not recognize that the attribute
has a default value, because "" is an empty TableGen string.

Strings no longer have special treatment. Instead, string values must be
wrapped in quotes: "\"foo\"". Two helpers, DefaultValuedStrAttr and
ConstantStrAttr have been added to keep code clean.

[rriddle]

mlir/tools/mlir-tblgen/RewriterGen.cpp

53–58

Why the cpp prefix?

199

Twine should be passed by const &

322

Same here.

[jpienaar]

Drive by :)

mlir/tools/mlir-tblgen/RewriterGen.cpp

199

Pass Twine as const Twine&

[Mogball]

Review comments

mlir/include/mlir/IR/OpBase.td

class OptionalAttr<Attr attr> : Attr<attr.predicate, attr.summary> {
   let convertFromStorage = "$_self ? " # returnType # "(" #
                            attr.convertFromStorage # ") : (::llvm::None)";
   let valueType = attr.valueType;
   let isOptional = 1;
 
   let baseAttr = attr;
 }
 
+// Default-valued string-based attribute. Wraps the default value in escaped
+// quotes.
+class DefaultValuedStrAttr<Attr attr, string val>
+    : DefaultValuedAttr<attr, "\"" # val # "\"">;
+
 //===----------------------------------------------------------------------===//
 // Primitive attribute kinds
 
 // A generic attribute that must be constructed around a specific buildable type
 // `attrValType`. Backed by MLIR attribute kind `attrKind`.
 class TypedAttrBase<Type attrValType, string attrKind, Pred condition,
                     string descr> :
     Attr<condition, descr> {
class FloatAttrBase<F attrValType, string descr> :
   let returnType = [{ ::llvm::APFloat }];
 }
 
 def F32Attr : FloatAttrBase<F32, "32-bit float attribute">;
 def F64Attr : FloatAttrBase<F64, "64-bit float attribute">;
 
 // An attribute backed by a string type.
 class StringBasedAttr<Pred condition, string descr> : Attr<condition, descr> {
-  let constBuilderCall = "$_builder.getStringAttr(\"$0\")";
+  let constBuilderCall = "$_builder.getStringAttr($0)";
   let storageType = [{ ::mlir::StringAttr }];
   let returnType = [{ ::llvm::StringRef }];
   let valueType = NoneType;
 }
 
 def StrAttr : StringBasedAttr<CPred<"$_self.isa<::mlir::StringAttr>()">,
                               "string attribute">;
 
class ConstantAttr<Attr attribute, string val> : AttrConstraint<
   string value = val;
 }
 
 class ConstF32Attr<string val> : ConstantAttr<F32Attr, val>;
 def ConstBoolAttrFalse : ConstantAttr<BoolAttr, "false">;
 def ConstBoolAttrTrue : ConstantAttr<BoolAttr, "true">;
 def ConstUnitAttr : ConstantAttr<UnitAttr, "unit">;
 
+// Constant string-based attribute. Wraps the desired string in escaped quotes.
+class ConstantStrAttr<Attr attribute, string val>
+    : ConstantAttr<attribute, "\"" # val # "\"">;
+
 //===----------------------------------------------------------------------===//
 // Common attribute constraints
 //===----------------------------------------------------------------------===//
 
 // A general mechanism to further confine the given `attr` with all the
 // `constraints`. This allows to compose complex constraints out of a series
 // of more primitive ones.
 class Confined<Attr attr, list<AttrConstraint> constraints> : Attr<

mlir/test/lib/Dialect/Test/TestOps.td

let extraClassDeclaration = [{
     }
 
     std::string getLibraryCallName() {
       return "";
     }
   }];
 }
 
+//===----------------------------------------------------------------------===//
+// Test Ops with Default-Valued String Attributes
+//===----------------------------------------------------------------------===//
+
+def TestDefaultStrAttrNoValueOp : TEST_Op<"no_str_value"> {
+  let arguments = (ins DefaultValuedAttr<StrAttr, "">:$value);
+  let assemblyFormat = "attr-dict";
+}
+
+def TestDefaultStrAttrHasValueOp : TEST_Op<"has_str_value"> {
+  let arguments = (ins DefaultValuedStrAttr<StrAttr, "">:$value);
+  let assemblyFormat = "attr-dict";
+}
+
+def : Pat<(TestDefaultStrAttrNoValueOp $value),
+          (TestDefaultStrAttrHasValueOp ConstantStrAttr<StrAttr, "foo">)>;
+
 #endif // TEST_OPS

mlir/test/mlir-tblgen/constant-str-attr-invalid.mlir

+// RUN: mlir-opt -verify-diagnostics %s
+
+// Test DefaultValuedAttr<StrAttr, ""> is recognized as "no default value"
+test.no_str_value {} // expected-error {{'test.no_str_value' op requires attribute 'value'}}

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

def DOp : NS_Op<"d_op", []> {
   let arguments = (ins
     I32Attr:$i32_attr,
     F64Attr:$f64_attr,
     StrAttr:$str_attr,
     BoolAttr:$bool_attr,
     SomeI32Enum:$enum_attr,
     DefaultValuedAttr<I32Attr, "42">:$dv_i32_attr,
     DefaultValuedAttr<F64Attr, "8.">:$dv_f64_attr,
-    DefaultValuedAttr<StrAttr, "abc">:$dv_str_attr,
+    DefaultValuedStrAttr<StrAttr, "abc">:$dv_str_attr,
     DefaultValuedAttr<BoolAttr, "true">:$dv_bool_attr,
     DefaultValuedAttr<SomeI32Enum, "::SomeI32Enum::case5">:$dv_enum_attr
   );
 }
 
 // DECL-LABEL: DOp declarations
 // DECL: static void build({{.*}}, uint32_t i32_attr, ::llvm::APFloat f64_attr, ::llvm::StringRef str_attr, bool bool_attr, ::SomeI32Enum enum_attr, uint32_t dv_i32_attr, ::llvm::APFloat dv_f64_attr, ::llvm::StringRef dv_str_attr = "abc", bool dv_bool_attr = true, ::SomeI32Enum dv_enum_attr = ::SomeI32Enum::case5)
 

 
 def EOp : NS_Op<"e_op", []> {
   let arguments = (ins
     I32Attr:$i32_attr,
     DefaultValuedAttr<I32Attr, "42">:$dv_i32_attr,
     F64Attr:$f64_attr,
     DefaultValuedAttr<F64Attr, "8.">:$dv_f64_attr,
     StrAttr:$str_attr,
-    DefaultValuedAttr<StrAttr, "abc">:$dv_str_attr,
+    DefaultValuedStrAttr<StrAttr, "abc">:$dv_str_attr,
     BoolAttr:$bool_attr,
     DefaultValuedAttr<BoolAttr, "true">:$dv_bool_attr,
     SomeI32Enum:$enum_attr,
     DefaultValuedAttr<SomeI32Enum, "::SomeI32Enum::case5">:$dv_enum_attr
   );
 }
 
 // DECL-LABEL: EOp declarations

mlir/test/mlir-tblgen/op-format.mlir

 %ignored_res6 = test.format_types_match_context %i64 : i64
 
 //===----------------------------------------------------------------------===//
 // InferTypeOpInterface type inference
 //===----------------------------------------------------------------------===//
 
 // CHECK: test.format_infer_type
 %ignored_res7 = test.format_infer_type
+
+//===----------------------------------------------------------------------===//
+// Check DefaultValuedStrAttr
+//===----------------------------------------------------------------------===//
+
+// CHECK: test.has_str_value
+test.has_str_value {}

mlir/test/mlir-tblgen/pattern.mlir

 
 func @returnTypeAndLocation(%arg0 : i32) -> i1 {
   %0 = "test.source_op"(%arg0) {tag = 66 : i32} : (i32) -> i1
   // CHECK: "test.op_x"(%arg0) : (i32) -> i32 loc("loc1")
   // CHECK: "test.op_x"(%arg0) : (i32) -> i32 loc("loc2")
   // CHECK: "test.two_to_one"(%0, %1) : (i32, i32) -> i1
   return %0 : i1
 }
+
+//===----------------------------------------------------------------------===//
+// Test that patterns can create ConstantStrAttr
+//===----------------------------------------------------------------------===//
+
+func @testConstantStrAttr() -> () {
+  // CHECK: test.has_str_value {value = "foo"}
+  test.no_str_value {value = "bar"}
+  return
+}

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

case AttrParamKind::UnwrappedValue:
         type = attr.getStorageType();
       break;
     }
 
     // Attach default value if requested and possible.
     std::string defaultValue;
     if (attrParamKind == AttrParamKind::UnwrappedValue &&
         i >= defaultValuedAttrStartIndex) {
-      bool isString = attr.getReturnType() == "::llvm::StringRef";
-      if (isString)
-        defaultValue.append("\"");
       defaultValue += attr.getDefaultValue();
-      if (isString)
-        defaultValue.append("\"");
     }
     paramList.emplace_back(type, namedAttr.name, defaultValue, properties);
   }
 
   /// Insert parameters for each successor.
   for (const NamedSuccessor &succ : op.getSuccessors()) {
     StringRef type =
         succ.isVariadic() ? "::mlir::BlockRange" : "::mlir::Block *";

mlir/tools/mlir-tblgen/RewriterGen.cpp

 struct format_provider<mlir::tblgen::Pattern::IdentifierLine> {
   static void format(const mlir::tblgen::Pattern::IdentifierLine &v,
                      raw_ostream &os, StringRef style) {
     os << v.first << ":" << v.second;
   }
 };
 } // end namespace llvm
 
+static std::string cppEscapeString(StringRef value) {
+  std::string ret;
+  llvm::raw_string_ostream os(ret);
+  llvm::printEscapedString(value, os);
+  return os.str();
+}
+
 //===----------------------------------------------------------------------===//
 // PatternEmitter
 //===----------------------------------------------------------------------===//
 
 namespace {
 
 class StaticMatcherHelper;
 
private:
 
   // Emits the local variables for holding all values as a whole and all named
   // attributes as a whole to be used for creating an op.
   void createAggregateLocalVarsForOpArgs(
       DagNode node, const ChildNodeIndexNameMap &childNodeNames, int depth);
 
   // Returns the C++ expression to construct a constant attribute of the given
   // `value` for the given attribute kind `attr`.
-  std::string handleConstantAttr(Attribute attr, StringRef value);
+  std::string handleConstantAttr(Attribute attr, Twine value);
 
   // Returns the C++ expression to build an argument from the given DAG `leaf`.
   // `patArgName` is used to bound the argument to the source pattern.
   std::string handleOpArgument(DagLeaf leaf, StringRef patArgName);
 
   //===--------------------------------------------------------------------===//
   // General utilities
   //===--------------------------------------------------------------------===//

 PatternEmitter::PatternEmitter(Record *pat, RecordOperatorMap *mapper,
                                raw_ostream &os, StaticMatcherHelper &helper)
     : loc(pat->getLoc()), opMap(mapper), pattern(pat, mapper),
       symbolInfoMap(pat->getLoc()), staticMatcherHelper(helper), nextValueId(0),
       os(os) {
   fmtCtx.withBuilder("rewriter");
 }
 
-std::string PatternEmitter::handleConstantAttr(Attribute attr,
-                                               StringRef value) {
+std::string PatternEmitter::handleConstantAttr(Attribute attr, Twine value) {
   if (!attr.isConstBuildable())
     PrintFatalError(loc, "Attribute " + attr.getAttrDefName() +
                              " does not have the 'constBuilderCall' field");
 
   // TODO: Verify the constants here
   return std::string(tgfmt(attr.getConstBuilderTemplate(), &fmtCtx, value));
 }
 
for (int i = 0, e = tree.getNumArgs() - tail.numDirectives; i != e; ++i) {
     else
       self = formatv("{0}.getType()", argName);
     emitMatchCheck(
         opName,
         tgfmt(constraint.getConditionTemplate(), &fmtCtx.withSelf(self)),
         formatv("\"operand {0} of native code call '{1}' failed to satisfy "
                 "constraint: "
                 "'{2}'\"",
-                i, tree.getNativeCodeTemplate(), constraint.getSummary()));
+                i, tree.getNativeCodeTemplate(),
+                cppEscapeString(constraint.getSummary())));
   }
 
   LLVM_DEBUG(llvm::dbgs() << "done emitting match for native code call\n");
 }
 
 // Helper function to match patterns.
 void PatternEmitter::emitOpMatch(DagNode tree, StringRef opName, int depth) {
   Operator &op = tree.getDialectOp(opMap);
if (operand->constraint != constraint) {
       auto self = formatv("(*{0}.getODSOperands({1}).begin()).getType()",
                           opName, operandIndex);
       emitMatchCheck(
           opName,
           tgfmt(constraint.getConditionTemplate(), &fmtCtx.withSelf(self)),
           formatv("\"operand {0} of op '{1}' failed to satisfy constraint: "
                   "'{2}'\"",
                   operand - op.operand_begin(), op.getOperationName(),
-                  constraint.getSummary()));
+                  cppEscapeString(constraint.getSummary())));
     }
   }
 
   // Capture the value
   auto name = tree.getArgName(argIndex);
   // `$_` is a special symbol to ignore op argument matching.
   if (!name.empty() && name != "_") {
     // We need to subtract the number of attributes before this operand to get
if (!matcher.isUnspecified()) {
     }
 
     // If a constraint is specified, we need to generate C++ statements to
     // check the constraint.
     emitMatchCheck(
         opName,
         tgfmt(matcher.getConditionTemplate(), &fmtCtx.withSelf("tblgen_attr")),
         formatv("\"op '{0}' attribute '{1}' failed to satisfy constraint: "
-                "{2}\"",
+                "'{2}'\"",
                 op.getOperationName(), namedAttr->name,
-                matcher.getAsConstraint().getSummary()));
+                cppEscapeString(matcher.getAsConstraint().getSummary())));
   }
 
   // Capture the value
   auto name = tree.getArgName(argIndex);
   // `$_` is a special symbol to ignore op argument matching.
   if (!name.empty() && name != "_") {
     os << formatv("{0} = tblgen_attr;\n", name);
   }
for (auto &appliedConstraint : pattern.getConstraints()) {
     auto &entities = appliedConstraint.entities;
 
     auto condition = constraint.getConditionTemplate();
     if (isa<TypeConstraint>(constraint)) {
       auto self = formatv("({0}.getType())",
                           symbolInfoMap.getValueAndRangeUse(entities.front()));
       emitMatchCheck(
           opName, tgfmt(condition, &fmtCtx.withSelf(self.str())),
-          formatv("\"value entity '{0}' failed to satisfy constraint: {1}\"",
-                  entities.front(), constraint.getSummary()));
+          formatv("\"value entity '{0}' failed to satisfy constraint: '{1}'\"",
+                  entities.front(), cppEscapeString(constraint.getSummary())));
 
     } else if (isa<AttrConstraint>(constraint)) {
       PrintFatalError(
           loc, "cannot use AttrConstraint in Pattern multi-entity constraints");
     } else {
       // TODO: replace formatv arguments with the exact specified
       // args.
       if (entities.size() > 4) {
         PrintFatalError(loc, "only support up to 4-entity constraints now");
       }
       SmallVector<std::string, 4> names;
       int i = 0;
       for (int e = entities.size(); i < e; ++i)
         names.push_back(symbolInfoMap.getValueAndRangeUse(entities[i]));
       std::string self = appliedConstraint.self;
       if (!self.empty())
         self = symbolInfoMap.getValueAndRangeUse(self);
       for (; i < 4; ++i)
         names.push_back("<unused>");
       emitMatchCheck(opName,
                      tgfmt(condition, &fmtCtx.withSelf(self), names[0],
                            names[1], names[2], names[3]),
                      formatv("\"entities '{0}' failed to satisfy constraint: "
-                             "{1}\"",
+                             "'{1}'\"",
                              llvm::join(entities, ", "),
-                             constraint.getSummary()));
+                             cppEscapeString(constraint.getSummary())));
     }
   }
 
   // Some of the operands could be bound to the same symbol name, we need
   // to enforce equality constraint on those.
   // TODO: we should be able to emit equality checks early
   // and short circuit unnecessary work if vars are not equal.
   for (auto symbolInfoIt = symbolInfoMap.begin();
std::string PatternEmitter::handleOpArgument(DagLeaf leaf,
   if (leaf.isConstantAttr()) {
     auto constAttr = leaf.getAsConstantAttr();
     return handleConstantAttr(constAttr.getAttribute(),
                               constAttr.getConstantValue());
   }
   if (leaf.isEnumAttrCase()) {
     auto enumCase = leaf.getAsEnumAttrCase();
     if (enumCase.isStrCase())
-      return handleConstantAttr(enumCase, enumCase.getSymbol());
+      return handleConstantAttr(enumCase, "\"" + enumCase.getSymbol() + "\"");
     // This is an enum case backed by an IntegerAttr. We need to get its value
     // to build the constant.
     std::string val = std::to_string(enumCase.getValue());
     return handleConstantAttr(enumCase, val);
   }
 
   LLVM_DEBUG(llvm::dbgs() << "handle argument '" << patArgName << "'\n");
   auto argName = symbolInfoMap.getValueAndRangeUse(patArgName);