Reimplement mlir::Identifier to be a wrapper around 'StringMapEntry*' instead of a wrapper around a 'const char*'. This makes it so strref() can be computed without calling strlen, which is more efficient and less error-prone. While here...

[lattner]

Summary

..., reimplement DenseMapInfo<mlir::Identifier>::getHashValue in terms of mlir::hash_value(Identifier).

Both of these improvements were suggested by River, thanks!

[rriddle]

Thanks, looks much better now.

mlir/include/mlir/IR/Identifier.h

94

nit: Can we change this to arg.getAsOpaquePointer() to avoid the pointer arithmetic of getKeyData.

[rriddle]

mlir/include/mlir/IR/Identifier.h

84–85

Can you also fix these while you are at it? These should really just be inline operators that compare the entry.

84–85

(By inline I mean in the class)

[lattner]

mlir/include/mlir/IR/Identifier.h

84–85

I changed these to compare the entry, and moved the Id/Id versions inline. However, we can't move the StringRef versions inline splitting the equality operators into two groups.

I think both are fine, but please give me an a +1 that this is what you wanted.

Incidentally, the is() method should go away now. I can take care of that cleanup in a separate patch.

[lattner]

Tidy up some things River requested.

[jpienaar]

Nice :)

[rriddle]

mlir/include/mlir/IR/Identifier.h

84–85

Yeah, that's what I meant. +1 thanks.

[rriddle]

mlir/include/mlir/IR/Identifier.h

84–85

(i.e. what you have looks good to me. I only meant the Identifier, Identifier ones)

[lattner]

cool thx!

mlir/include/mlir/IR/Identifier.h

 //===- Identifier.h - MLIR Identifier Class ---------------------*- C++ -*-===//
 //
 // Part of the LLVM 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef MLIR_IR_IDENTIFIER_H
 #define MLIR_IR_IDENTIFIER_H
 
 #include "mlir/Support/LLVM.h"
 #include "llvm/ADT/DenseMapInfo.h"
-#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringMapEntry.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 
 namespace mlir {
 class MLIRContext;
 
 /// This class represents a uniqued string owned by an MLIRContext.  Strings
 /// represented by this type cannot contain nul characters, and may not have a
 /// zero length.
 ///
 /// This is a POD type with pointer size, so it should be passed around by
 /// value.  The underlying data is owned by MLIRContext and is thus immortal for
 /// almost all clients.
 class Identifier {
+  using EntryType = llvm::StringMapEntry<llvm::NoneType>;
+
 public:
   /// Return an identifier for the specified string.
   static Identifier get(StringRef str, MLIRContext *context);
   Identifier(const Identifier &) = default;
   Identifier &operator=(const Identifier &other) = default;
 
   /// Return a StringRef for the string.
-  StringRef strref() const { return StringRef(pointer, size()); }
+  StringRef strref() const { return entry->first(); }
 
   /// Identifiers implicitly convert to StringRefs.
   operator StringRef() const { return strref(); }
 
   /// Return an std::string.
   std::string str() const { return strref().str(); }
 
   /// Return a null terminated C string.
-  const char *c_str() const { return pointer; }
+  const char *c_str() const { return entry->getKeyData(); }
 
   /// Return a pointer to the start of the string data.
-  const char *data() const { return pointer; }
+  const char *data() const { return entry->getKeyData(); }
 
   /// Return the number of bytes in this string.
-  unsigned size() const { return ::strlen(pointer); }
+  unsigned size() const { return entry->getKeyLength(); }
 
   /// Return true if this identifier is the specified string.
-  bool is(StringRef string) const {
-    // Note: this can't use memcmp, because memcmp doesn't guarantee that it
-    // will stop reading both buffers if one is shorter than the other.
-    return strncmp(pointer, string.data(), string.size()) == 0 &&
-           pointer[string.size()] == '\0';
-  }
+  bool is(StringRef string) const { return strref() == string; }
 
-  const char *begin() const { return pointer; }
-  const char *end() const { return pointer + size(); }
+  const char *begin() const { return data(); }
+  const char *end() const { return entry->getKeyData() + size(); }
+
+  bool operator==(Identifier other) const { return entry == other.entry; }
+  bool operator!=(Identifier rhs) const { return !(*this == rhs); }
 
   void print(raw_ostream &os) const;
   void dump() const;
 
   const void *getAsOpaquePointer() const {
-    return static_cast<const void *>(pointer);
+    return static_cast<const void *>(entry);
   }
-  static Identifier getFromOpaquePointer(const void *pointer) {
-    return Identifier((const char *)pointer);
+  static Identifier getFromOpaquePointer(const void *entry) {
+    return Identifier(static_cast<const EntryType *>(entry));
   }
 
 private:
-  /// These are the bytes of the string, which is a nul terminated string.
-  const char *pointer;
-  explicit Identifier(const char *pointer) : pointer(pointer) {}
+  /// 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) {
   identifier.print(os);
   return os;
 }
 
-inline bool operator==(Identifier lhs, Identifier rhs) {
-  return lhs.data() == rhs.data();
-}
-
-inline bool operator!=(Identifier lhs, Identifier rhs) {
-  return lhs.data() != rhs.data();
-}
-
+// Identifier/Identifier equality comparisons are defined inline.
 inline bool operator==(Identifier lhs, StringRef rhs) { return lhs.is(rhs); }
 inline bool operator!=(Identifier lhs, StringRef rhs) { return !lhs.is(rhs); }
 inline bool operator==(StringRef lhs, Identifier rhs) { return rhs.is(lhs); }
 inline bool operator!=(StringRef lhs, Identifier rhs) { return !rhs.is(lhs); }
 
 // Make identifiers hashable.
 inline llvm::hash_code hash_value(Identifier arg) {
   // Identifiers are uniqued, so we can just hash the pointer they contain.
-  return llvm::hash_value(static_cast<const void *>(arg.data()));
+  return llvm::hash_value(arg.getAsOpaquePointer());
 }
 } // end namespace mlir
 
 namespace llvm {
 // Identifiers hash just like pointers, there is no need to hash the bytes.
 template <>
 struct DenseMapInfo<mlir::Identifier> {
   static mlir::Identifier getEmptyKey() {
     auto pointer = llvm::DenseMapInfo<const void *>::getEmptyKey();
     return mlir::Identifier::getFromOpaquePointer(pointer);
   }
   static mlir::Identifier getTombstoneKey() {
     auto pointer = llvm::DenseMapInfo<const void *>::getTombstoneKey();
     return mlir::Identifier::getFromOpaquePointer(pointer);
   }
   static unsigned getHashValue(mlir::Identifier val) {
-    return DenseMapInfo<const void *>::getHashValue(val.data());
+    return mlir::hash_value(val);
   }
   static bool isEqual(mlir::Identifier lhs, mlir::Identifier rhs) {
     return lhs == rhs;
   }
 };
 
 /// The pointer inside of an identifier comes from a StringMap, so its alignment
 /// is always at least 4 and probably 8 (on 64-bit machines).  Allow LLVM to

mlir/lib/IR/MLIRContext.cpp

 /// Return an identifier for the specified string.
 Identifier Identifier::get(StringRef str, MLIRContext *context) {
   auto &impl = context->getImpl();
 
   { // Check for an existing identifier in read-only mode.
     llvm::sys::SmartScopedReader<true> contextLock(impl.identifierMutex);
     auto it = impl.identifiers.find(str);
     if (it != impl.identifiers.end())
-      return Identifier(it->getKeyData());
+      return Identifier(&*it);
   }
 
   // Check invariants after seeing if we already have something in the
   // identifier table - if we already had it in the table, then it already
   // passed invariant checks.
   assert(!str.empty() && "Cannot create an empty identifier");
   assert(str.find('\0') == StringRef::npos &&
          "Cannot create an identifier with a nul character");
 
   // Acquire a writer-lock so that we can safely create the new instance.
   llvm::sys::SmartScopedWriter<true> contextLock(impl.identifierMutex);
   auto it = impl.identifiers.insert(str).first;
-  return Identifier(it->getKeyData());
+  return Identifier(&*it);
 }
 
 //===----------------------------------------------------------------------===//
 // Type uniquing
 //===----------------------------------------------------------------------===//
 
 static Dialect &lookupDialectForSymbol(MLIRContext *ctx, TypeID typeID) {
   auto &impl = ctx->getImpl();