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

`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.

Reviewed By: rriddle

Differential Revision: https://reviews.llvm.org/D111855

mlir/include/mlir/IR/OpBase.td

@@ -928,6 +928,11 @@ class OptionalAttr<Attr attr> : Attr<attr.predicate, attr.summary> {
   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
 
@@ -1095,7 +1100,7 @@ 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;
@@ -1672,6 +1677,10 @@ 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
 //===----------------------------------------------------------------------===//

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

@@ -2343,4 +2343,21 @@ def TestLinalgConvOp :
   }];
 }
 
+//===----------------------------------------------------------------------===//
+// 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

@@ -0,0 +1,4 @@
+// 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

@@ -337,7 +337,7 @@ def DOp : NS_Op<"d_op", []> {
     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
   );
@@ -377,7 +377,7 @@ def EOp : NS_Op<"e_op", []> {
     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,

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

@@ -361,3 +361,10 @@ test.format_infer_variadic_type_from_non_variadic %i64, %i64 : i64
 
 // 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

@@ -583,3 +583,13 @@ func @returnTypeAndLocation(%arg0 : i32) -> i1 {
   // 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

@@ -1710,12 +1710,7 @@ void OpEmitter::buildParamList(SmallVectorImpl<OpMethodParameter> &paramList,
     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);
   }

mlir/tools/mlir-tblgen/RewriterGen.cpp

@@ -50,6 +50,13 @@ struct format_provider<mlir::tblgen::Pattern::IdentifierLine> {
 };
 } // end namespace llvm
 
+static std::string escapeString(StringRef value) {
+  std::string ret;
+  llvm::raw_string_ostream os(ret);
+  llvm::printEscapedString(value, os);
+  return os.str();
+}
+
 //===----------------------------------------------------------------------===//
 // PatternEmitter
 //===----------------------------------------------------------------------===//
@@ -189,7 +196,7 @@ private:
 
   // 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, const 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.
@@ -313,7 +320,7 @@ PatternEmitter::PatternEmitter(Record *pat, RecordOperatorMap *mapper,
 }
 
 std::string PatternEmitter::handleConstantAttr(Attribute attr,
-                                               StringRef value) {
+                                               const Twine &value) {
   if (!attr.isConstBuildable())
     PrintFatalError(loc, "Attribute " + attr.getAttrDefName() +
                              " does not have the 'constBuilderCall' field");
@@ -492,7 +499,8 @@ void PatternEmitter::emitNativeCodeMatch(DagNode tree, StringRef opName,
         formatv("\"operand {0} of native code call '{1}' failed to satisfy "
                 "constraint: "
                 "'{2}'\"",
-                i, tree.getNativeCodeTemplate(), constraint.getSummary()));
+                i, tree.getNativeCodeTemplate(),
+                escapeString(constraint.getSummary())));
   }
 
   LLVM_DEBUG(llvm::dbgs() << "done emitting match for native code call\n");
@@ -630,7 +638,7 @@ void PatternEmitter::emitOperandMatch(DagNode tree, StringRef opName,
           formatv("\"operand {0} of op '{1}' failed to satisfy constraint: "
                   "'{2}'\"",
                   operand - op.operand_begin(), op.getOperationName(),
-                  constraint.getSummary()));
+                  escapeString(constraint.getSummary())));
     }
   }
 
@@ -694,9 +702,9 @@ void PatternEmitter::emitAttributeMatch(DagNode tree, StringRef opName,
         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()));
+                escapeString(matcher.getAsConstraint().getSummary())));
   }
 
   // Capture the value
@@ -740,8 +748,8 @@ void PatternEmitter::emitMatchLogic(DagNode tree, StringRef opName) {
                           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(), escapeString(constraint.getSummary())));
 
     } else if (isa<AttrConstraint>(constraint)) {
       PrintFatalError(
@@ -765,9 +773,9 @@ void PatternEmitter::emitMatchLogic(DagNode tree, StringRef 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()));
+                             escapeString(constraint.getSummary())));
     }
   }
 
@@ -1103,7 +1111,7 @@ std::string PatternEmitter::handleOpArgument(DagLeaf leaf,
   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());