[mlir][ODS] Use StringLiteral instead of StringRef when applicable

[vinograd47]

Summary

Use StringLiteral for function return type if it is known to return
constant string literals only.

This will make it visible to API users, that such values can be safely
stored, since they refers to constant data, which will never be deallocated.

StringRef is general is not safe to store for a long term,
since it might refer to temporal data allocated in heap.

Add inline and constexpr methods support to OpMethod.

[vinograd47]

This is an updated version of https://reviews.llvm.org/D95945 with attempt to fix build issues caused StringLiteral class usage. @mehdi_amini @antiagainst @bkramer Could you please take a look one more time?

Sorry, I didn't find a way to update existing Differential, Archanist refuses to update Closed change.

[vinograd47]

Update TypeDef - getMnemonic method.

[mehdi_amini]

LG

Sorry, I didn't find a way to update existing Differential, Archanist refuses to update Closed change.

You need to reopen the revision in the "add action": the same menu available above the comment box where you can approve a revision.

[vinograd47]

Use explicit StringLiteral ctor call.

[vinograd47]

@antiagainst Could you please take a look? Will this version work with current StringLiteral limitations?

[vinograd47]

Rebased

[jpienaar]

Is this something that we want to make a convention, then we'd want to then capture in https://mlir.llvm.org/getting_started/DeveloperGuide/ (I don't believe it is a LLVM convention)

[vinograd47]

In D97390#2597664, @jpienaar wrote:

Is this something that we want to make a convention, then we'd want to then capture in https://mlir.llvm.org/getting_started/DeveloperGuide/ (I don't believe it is a LLVM convention)

I don't think it is some kind of convention. I've used StringLiteral in the above places just to make it clear that the return value has infinite life time and it is safe to store it (in contrast to generic StringRef, which might point to temporally allocated data). It is not a requirement for the entire code base, just specific obvious places.

[vinograd47]

Rebased

mlir/include/mlir/TableGen/OpClass.h

public:
   // Properties (qualifiers) of class methods. Bitfield is used here to help
   // querying properties.
   enum Property {
     MP_None = 0x0,
     MP_Static = 0x1,
     MP_Constructor = 0x2,
     MP_Private = 0x4,
     MP_Declaration = 0x8,
+    MP_Inline = 0x10,
+    MP_Constexpr = 0x20 | MP_Inline,
     MP_StaticDeclaration = MP_Static | MP_Declaration,
   };
 
   template <typename... Args>
   OpMethod(StringRef retType, StringRef name, Property property, unsigned id,
            Args &&...args)
       : properties(property),
         methodSignature(retType, name, std::forward<Args>(args)...),
         methodBody(properties & MP_Declaration), id(id) {}
 
   OpMethod(OpMethod &&) = default;
 
   virtual ~OpMethod() = default;
 
   OpMethodBody &body() { return methodBody; }
 
   // Returns true if this is a static method.
   bool isStatic() const { return properties & MP_Static; }
 
   // Returns true if this is a private method.
   bool isPrivate() const { return properties & MP_Private; }
 
+  // Returns true if this is an inline method.
+  bool isInline() const { return properties & MP_Inline; }
+
   // Returns the name of this method.
   StringRef getName() const { return methodSignature.getName(); }
 
   // Returns the ID for this method
   unsigned getID() const { return id; }
 
   // Returns if this method makes the `other` method redundant.
   bool makesRedundant(const OpMethod &other) const {

mlir/lib/TableGen/OpClass.cpp

 //===----------------------------------------------------------------------===//
 // OpMethod definitions
 //===----------------------------------------------------------------------===//
 
 void OpMethod::writeDeclTo(raw_ostream &os) const {
   os.indent(2);
   if (isStatic())
     os << "static ";
+  if (properties & MP_Constexpr)
+    os << "constexpr ";
   methodSignature.writeDeclTo(os);
+  if (!isInline())
     os << ";";
+  else {
+    os << " {\n";
+    methodBody.writeTo(os);
+    os << "}";
+  }
 }
 
 void OpMethod::writeDefTo(raw_ostream &os, StringRef namePrefix) const {
   // Do not write definition if the method is decl only.
   if (properties & MP_Declaration)
     return;
+  // Do not generate separate definition for inline method
+  if (isInline())
+    return;
   methodSignature.writeDefTo(os, namePrefix);
   os << " {\n";
   methodBody.writeTo(os);
   os << "}";
 }
 
 //===----------------------------------------------------------------------===//
 // OpConstructor definitions

mlir/test/mlir-tblgen/op-decl-and-defs.td

 // CHECK:   ::mlir::ValueRange odsOperands;
 // CHECK: };
 
 // CHECK: class AOp : public ::mlir::Op<AOp, ::mlir::OpTrait::AtLeastNRegions<1>::Impl, ::mlir::OpTrait::AtLeastNResults<1>::Impl, ::mlir::OpTrait::ZeroSuccessor, ::mlir::OpTrait::AtLeastNOperands<1>::Impl, ::mlir::OpTrait::IsIsolatedFromAbove
 // CHECK-NOT: ::mlir::OpTrait::IsIsolatedFromAbove
 // CHECK: public:
 // CHECK:   using Op::Op;
 // CHECK:   using Adaptor = AOpAdaptor;
-// CHECK:   static ::llvm::StringRef getOperationName();
+// CHECK:   static constexpr ::llvm::StringLiteral getOperationName() {
+// CHECK:     return ::llvm::StringLiteral("test.a_op");
+// CHECK:   }
 // CHECK:   ::mlir::Operation::operand_range getODSOperands(unsigned index);
 // CHECK:   ::mlir::Value a();
 // CHECK:   ::mlir::Operation::operand_range b();
 // CHECK:   ::mlir::MutableOperandRange aMutable();
 // CHECK:   ::mlir::MutableOperandRange bMutable();
 // CHECK:   ::mlir::Operation::result_range getODSResults(unsigned index);
 // CHECK:   ::mlir::Value r();
 // CHECK:   ::mlir::Region &someRegion();

mlir/test/mlir-tblgen/typedefs.td

let parameters = (
       RTLValueType:$inner
   );
 
   let genVerifyDecl = 1;
 
 // DECL-LABEL: class CompoundAType : public ::mlir::Type
 // DECL: static CompoundAType getChecked(llvm::function_ref<::mlir::InFlightDiagnostic()> emitError, ::mlir::MLIRContext *context, int widthOfSomething, ::mlir::test::SimpleTypeA exampleTdType, SomeCppStruct exampleCppType, ::llvm::ArrayRef<int> dims, ::mlir::Type inner);
 // DECL: static ::mlir::LogicalResult verify(::llvm::function_ref<::mlir::InFlightDiagnostic()> emitError, int widthOfSomething, ::mlir::test::SimpleTypeA exampleTdType, SomeCppStruct exampleCppType, ::llvm::ArrayRef<int> dims, ::mlir::Type inner);
-// DECL: static ::llvm::StringRef getMnemonic() { return "cmpnd_a"; }
+// DECL: static constexpr ::llvm::StringLiteral getMnemonic() {
+// DECL:   return ::llvm::StringLiteral("cmpnd_a");
+// DECL: }
 // DECL: static ::mlir::Type parse(::mlir::MLIRContext *context,
 // DECL-NEXT: ::mlir::DialectAsmParser &parser);
 // DECL: void print(::mlir::DialectAsmPrinter &printer) const;
 // DECL: int getWidthOfSomething() const;
 // DECL: ::mlir::test::SimpleTypeA getExampleTdType() const;
 // DECL: SomeCppStruct getExampleCppType() const;
 }
 
 def C_IndexType : TestType<"Index"> {
     let mnemonic = "index";
 
     let parameters = (
       ins
       StringRefParameter<"Label for index">:$label
     );
 
 // DECL-LABEL: class IndexType : public ::mlir::Type
-// DECL: static ::llvm::StringRef getMnemonic() { return "index"; }
+// DECL: static constexpr ::llvm::StringLiteral getMnemonic() {
+// DECL:   return ::llvm::StringLiteral("index");
+// DECL: }
 // DECL: static ::mlir::Type parse(::mlir::MLIRContext *context,
 // DECL-NEXT: ::mlir::DialectAsmParser &parser);
 // DECL: void print(::mlir::DialectAsmPrinter &printer) const;
 }
 
 def D_SingleParameterType : TestType<"SingleParameter"> {
   let parameters = (
     ins

mlir/tools/mlir-tblgen/DialectGen.cpp

explicit {0}(::mlir::MLIRContext *context)
     : ::mlir::Dialect(getDialectNamespace(), context,
       ::mlir::TypeID::get<{0}>()) {{
     {2}
     initialize();
   }
   void initialize();
   friend class ::mlir::MLIRContext;
 public:
-  static ::llvm::StringRef getDialectNamespace() { return "{1}"; }
+  static constexpr ::llvm::StringLiteral getDialectNamespace() {
+    return ::llvm::StringLiteral("{1}");
+  }
 )";
 
 /// Registration for a single dependent dialect: to be inserted in the ctor
 /// above for each dependent dialect.
 const char *const dialectRegistrationTemplate = R"(
     getContext()->getOrLoadDialect<{0}>();
 )";
 

mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp

if (auto opTrait = dyn_cast<tblgen::NativeOpTrait>(&trait))
       opClass.addTrait(opTrait->getTrait());
     else if (auto opTrait = dyn_cast<tblgen::InterfaceOpTrait>(&trait))
       opClass.addTrait(opTrait->getTrait());
   }
 }
 
 void OpEmitter::genOpNameGetter() {
   auto *method = opClass.addMethodAndPrune(
-      "::llvm::StringRef", "getOperationName", OpMethod::MP_Static);
-  method->body() << "  return \"" << op.getOperationName() << "\";\n";
+      "::llvm::StringLiteral", "getOperationName",
+      OpMethod::Property(OpMethod::MP_Static | OpMethod::MP_Constexpr));
+  method->body() << "  return ::llvm::StringLiteral(\"" << op.getOperationName()
+                 << "\");";
 }
 
 void OpEmitter::genOpAsmInterface() {
   // If the user only has one results or specifically added the Asm trait,
   // then don't generate it for them. We specifically only handle multi result
   // operations, because the name of a single result in the common case is not
   // interesting(generally 'result'/'output'/etc.).
   // TODO: We could also add a flag to allow operations to opt in to this

mlir/tools/mlir-tblgen/PassGen.cpp

 const char *const passDeclBegin = R"(
 //===----------------------------------------------------------------------===//
 // {0}
 //===----------------------------------------------------------------------===//
 
 template <typename DerivedT>
 class {0}Base : public {1} {
 public:
+  using Base = {0}Base;
+
   {0}Base() : {1}(::mlir::TypeID::get<DerivedT>()) {{}
   {0}Base(const {0}Base &) : {1}(::mlir::TypeID::get<DerivedT>()) {{}
 
   /// Returns the command-line argument attached to this pass.
+  static constexpr ::llvm::StringLiteral getArgumentName() {
+    return ::llvm::StringLiteral("{2}");
+  }
   ::llvm::StringRef getArgument() const override { return "{2}"; }
 
   /// Returns the derived pass name.
+  static constexpr ::llvm::StringLiteral getPassName() {
+    return ::llvm::StringLiteral("{0}");
+  }
   ::llvm::StringRef getName() const override { return "{0}"; }
 
   /// Support isa/dyn_cast functionality for the derived pass class.
   static bool classof(const ::mlir::Pass *pass) {{
     return pass->getTypeID() == ::mlir::TypeID::get<DerivedT>();
   }
 
   /// A clone method to create a copy of this pass.

mlir/tools/mlir-tblgen/TypeDefGen.cpp

if (!params.empty()) {
 
     // Emit the verify invariants declaration.
     if (typeDef.genVerifyDecl())
       os << llvm::formatv(typeDefDeclVerifyStr, emitTypeNamePairsAfterComma);
   }
 
   // Emit the mnenomic, if specified.
   if (auto mnenomic = typeDef.getMnemonic()) {
-    os << "    static ::llvm::StringRef getMnemonic() { return \"" << mnenomic
-       << "\"; }\n";
+    os << "    static constexpr ::llvm::StringLiteral getMnemonic() {\n"
+       << "      return ::llvm::StringLiteral(\"" << mnenomic << "\");\n"
+       << "    }\n";
 
     // If mnemonic specified, emit print/parse declarations.
     if (typeDef.getParserCode() || typeDef.getPrinterCode() || !params.empty())
       os << typeDefParsePrint;
   }
 
   if (typeDef.genAccessors()) {
     SmallVector<TypeParameter, 4> parameters;