Store an Identifier instead of a StringRef for the OperationName inside an AbstractOperation (NFC)

[mehdi_amini]

Summary

Instead of storing a StringRef, we keep an Identifier which otherwise requires a lock on the context to retrieve.
This will allow to get an Identifier for any registered Operation for "free".

[rriddle]

mlir/include/mlir/IR/Identifier.h

71

Naming here should be camelCase.

mlir/include/mlir/IR/Operation.h

61

I don't think we should provide this, it can be easily accessed from the OperationName.

[mehdi_amini]

Address comments

mlir/include/mlir/IR/Identifier.h

public:
 
   const void *getAsOpaquePointer() const {
     return static_cast<const void *>(entry);
   }
   static Identifier getFromOpaquePointer(const void *entry) {
     return Identifier(static_cast<const EntryType *>(entry));
   }
 
+  /// Compare the underlying StringRef.
+  int compare(Identifier RHS) const { return strref().compare(RHS.strref()); }
+
 private:
   /// This contains the bytes of the string, which is guaranteed to be nul
   /// terminated.
   const EntryType *entry;
   explicit Identifier(const EntryType *entry) : entry(entry) {}
 };
 
 inline raw_ostream &operator<<(raw_ostream &os, Identifier identifier) {

mlir/include/mlir/IR/Operation.h

static Operation *create(Location location, OperationName name,
                            ArrayRef<Type> resultTypes, ArrayRef<Value> operands,
                            MutableDictionaryAttr attributes,
                            ArrayRef<Block *> successors = {},
                            RegionRange regions = {});
 
   /// The name of an operation is the key identifier for it.
   OperationName getName() { return name; }
 
+  /// The name of an operation as an Identifier: this provides a reference to
+  /// the uniqued internalized string in the context.
+  Identifier getIdentifier() { return getName().getIdentifier(); }
+
   /// If this operation has a registered operation description, return it.
   /// Otherwise return null.
   const AbstractOperation *getAbstractOperation() {
     return getName().getAbstractOperation();
   }
 
   /// Returns true if this operation has a registered operation description,
   /// otherwise false.

mlir/include/mlir/IR/OperationSupport.h

 /// should only be used by things like the AsmPrinter and other things that need
 /// to be parameterized by generic operation hooks.  Most user code should use
 /// the concrete operation types.
 class AbstractOperation {
 public:
   using OperationProperties = uint32_t;
 
   /// This is the name of the operation.
-  const StringRef name;
+  const Identifier name;
 
   /// This is the dialect that this operation belongs to.
   Dialect &dialect;
 
   /// The unique identifier of the derived Op class.
   TypeID typeID;
 
   /// Use the specified object to parse this ops custom assembly format.
AbstractOperation(
       TypeID typeID,
       ParseResult (&parseAssembly)(OpAsmParser &parser, OperationState &result),
       void (&printAssembly)(Operation *op, OpAsmPrinter &p),
       LogicalResult (&verifyInvariants)(Operation *op),
       LogicalResult (&foldHook)(Operation *op, ArrayRef<Attribute> operands,
                                 SmallVectorImpl<OpFoldResult> &results),
       void (&getCanonicalizationPatterns)(OwningRewritePatternList &results,
                                           MLIRContext *context),
-      detail::InterfaceMap &&interfaceMap, bool (&hasTrait)(TypeID traitID))
-      : name(name), dialect(dialect), typeID(typeID),
-        parseAssembly(parseAssembly), printAssembly(printAssembly),
-        verifyInvariants(verifyInvariants), foldHook(foldHook),
-        getCanonicalizationPatterns(getCanonicalizationPatterns),
-        opProperties(opProperties), interfaceMap(std::move(interfaceMap)),
-        hasRawTrait(hasTrait) {}
+      detail::InterfaceMap &&interfaceMap, bool (&hasTrait)(TypeID traitID));
 
   /// The properties of the operation.
   const OperationProperties opProperties;
 
   /// A map of interfaces that were registered to this operation.
   detail::InterfaceMap interfaceMap;
 
   /// This hook returns if the operation contains the trait corresponding
public:
   OperationName(StringRef name, MLIRContext *context);
 
   /// Return the name of the dialect this operation is registered to.
   StringRef getDialect() const;
 
   /// Return the operation name with dialect name stripped, if it has one.
   StringRef stripDialect() const;
 
-  /// Return the name of this operation.  This always succeeds.
+  /// Return the name of this operation. This always succeeds.
   StringRef getStringRef() const;
 
+  /// Return the name of this operation as an identifier. This always succeeds.
+  Identifier getIdentifier() const;
+
   /// If this operation has a registered operation description, return it.
   /// Otherwise return null.
   const AbstractOperation *getAbstractOperation() const;
 
   void print(raw_ostream &os) const;
   void dump() const;
 
   void *getAsOpaquePointer() const {

mlir/lib/IR/MLIRContext.cpp

const AbstractOperation *AbstractOperation::lookup(StringRef opName,
                                                    MLIRContext *context) {
   auto &impl = context->getImpl();
   auto it = impl.registeredOperations.find(opName);
   if (it != impl.registeredOperations.end())
     return &it->second;
   return nullptr;
 }
 
+AbstractOperation::AbstractOperation(
+    StringRef name, Dialect &dialect, OperationProperties opProperties,
+    TypeID typeID,
+    ParseResult (&parseAssembly)(OpAsmParser &parser, OperationState &result),
+    void (&printAssembly)(Operation *op, OpAsmPrinter &p),
+    LogicalResult (&verifyInvariants)(Operation *op),
+    LogicalResult (&foldHook)(Operation *op, ArrayRef<Attribute> operands,
+                              SmallVectorImpl<OpFoldResult> &results),
+    void (&getCanonicalizationPatterns)(OwningRewritePatternList &results,
+                                        MLIRContext *context),
+    detail::InterfaceMap &&interfaceMap, bool (&hasTrait)(TypeID traitID))
+    : name(Identifier::get(name, dialect.getContext())), dialect(dialect),
+      typeID(typeID), parseAssembly(parseAssembly),
+      printAssembly(printAssembly), verifyInvariants(verifyInvariants),
+      foldHook(foldHook),
+      getCanonicalizationPatterns(getCanonicalizationPatterns),
+      opProperties(opProperties), interfaceMap(std::move(interfaceMap)),
+      hasRawTrait(hasTrait) {}
+
 /// Get the dialect that registered the type with the provided typeid.
 const AbstractType &AbstractType::lookup(TypeID typeID, MLIRContext *context) {
   auto &impl = context->getImpl();
   auto it = impl.registeredTypes.find(typeID);
   if (it == impl.registeredTypes.end())
     llvm::report_fatal_error(
         "Trying to create a Type that was not registered in this MLIRContext.");
   return *it->second;

mlir/lib/IR/Operation.cpp

 }
 
 /// Return the operation name with dialect name stripped, if it has one.
 StringRef OperationName::stripDialect() const {
   auto splitName = getStringRef().split(".");
   return splitName.second.empty() ? splitName.first : splitName.second;
 }
 
-/// Return the name of this operation.  This always succeeds.
+/// Return the name of this operation. This always succeeds.
 StringRef OperationName::getStringRef() const {
+  return getIdentifier().strref();
+}
+
+/// Return the name of this operation as an identifier. This always succeeds.
+Identifier OperationName::getIdentifier() const {
   if (auto *op = representation.dyn_cast<const AbstractOperation *>())
     return op->name;
-  return representation.get<Identifier>().strref();
+  return representation.get<Identifier>();
 }
 
 const AbstractOperation *OperationName::getAbstractOperation() const {
   return representation.dyn_cast<const AbstractOperation *>();
 }
 
 OperationName OperationName::getFromOpaquePointer(void *pointer) {
   return OperationName(RepresentationUnion::getFromOpaqueValue(pointer));

mlir/lib/Parser/Parser.cpp

public:
   //===--------------------------------------------------------------------===//
 
   /// Return if any errors were emitted during parsing.
   bool didEmitError() const { return emittedError; }
 
   /// Emit a diagnostic at the specified location and return failure.
   InFlightDiagnostic emitError(llvm::SMLoc loc, const Twine &message) override {
     emittedError = true;
-    return parser.emitError(loc, "custom op '" + opDefinition->name + "' " +
-                                     message);
+    return parser.emitError(loc, "custom op '" + opDefinition->name.strref() +
+                                     "' " + message);
   }
 
   llvm::SMLoc getCurrentLocation() override {
     return parser.getToken().getLoc();
   }
 
   Builder &getBuilder() const override { return parser.builder; }