Implement some micro-optimizations for Identifier. NFC

Summary:
Identifier doesn't maintain a length, so every time strref() is called,
it does a strlen.  In the case of comparisons, this isn't necessary:
there is no need to scan a string to get its length, then rescan it to
do the comparison.  Just done one comparison.

This also moves some assertions in Identifier::get as another
microoptimization for 'assertions enabled' modes.

Reviewers: rriddle!

Subscribers: mehdi_amini, rriddle, jpienaar, burmako, shauheen, antiagainst, nicolasvasilache, arpith-jacob, mgester, lucyrfox, liufengdb, Joonsoo, grosul1, frgossen, llvm-commits

Tags: #llvm

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

diff --git a/mlir/include/mlir/IR/Identifier.h b/mlir/include/mlir/IR/Identifier.h
index f3aead4..fae92fa 100644 (file)
--- a/mlir/include/mlir/IR/Identifier.h
+++ b/mlir/include/mlir/IR/Identifier.h
@@ -50,7 +50,12 @@ public:
   unsigned size() const { return ::strlen(pointer); }
 
   /// Return true if this identifier is the specified string.
-  bool is(StringRef string) const { return strref().equals(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';
+  }
 
   const char *begin() const { return pointer; }
   const char *end() const { return pointer + size(); }

diff --git a/mlir/lib/IR/Attributes.cpp b/mlir/lib/IR/Attributes.cpp
index 4526d7d..6829e8a 100644 (file)
--- a/mlir/lib/IR/Attributes.cpp
+++ b/mlir/lib/IR/Attributes.cpp
@@ -87,7 +87,7 @@ bool BoolAttr::getValue() const { return getImpl()->value; }
 /// NamedAttributes.
 static int compareNamedAttributes(const NamedAttribute *lhs,
                                   const NamedAttribute *rhs) {
-  return lhs->first.strref().compare(rhs->first.strref());
+  return strcmp(lhs->first.data(), rhs->first.data());
 }
 
 DictionaryAttr DictionaryAttr::get(ArrayRef<NamedAttribute> value,
@@ -111,7 +111,7 @@ DictionaryAttr DictionaryAttr::get(ArrayRef<NamedAttribute> value,
            "DictionaryAttr element names must be unique");
 
     // Don't invoke a general sort for two element case.
-    if (value[0].first.strref() > value[1].first.strref()) {
+    if (compareNamedAttributes(&value[0], &value[1]) > 0) {
       storage.push_back(value[1]);
       storage.push_back(value[0]);
       value = storage;
@@ -121,7 +121,7 @@ DictionaryAttr DictionaryAttr::get(ArrayRef<NamedAttribute> value,
     // Check to see they are sorted already.
     bool isSorted = true;
     for (unsigned i = 0, e = value.size() - 1; i != e; ++i) {
-      if (value[i].first.strref() > value[i + 1].first.strref()) {
+      if (compareNamedAttributes(&value[i], &value[i + 1]) > 0) {
         isSorted = false;
         break;
       }
@@ -152,8 +152,13 @@ ArrayRef<NamedAttribute> DictionaryAttr::getValue() const {
 /// Return the specified attribute if present, null otherwise.
 Attribute DictionaryAttr::get(StringRef name) const {
   ArrayRef<NamedAttribute> values = getValue();
-  auto compare = [](NamedAttribute attr, StringRef name) {
-    return attr.first.strref() < name;
+  auto compare = [](NamedAttribute attr, StringRef name) -> bool {
+    // This is correct even when attr.first.data()[name.size()] is not a zero
+    // string terminator, because we only care about a less than comparison.
+    // This can't use memcmp, because it doesn't guarantee that it will stop
+    // reading both buffers if one is shorter than the other, even if there is
+    // a difference.
+    return strncmp(attr.first.data(), name.data(), name.size()) < 0;
   };
   auto it = llvm::lower_bound(values, name, compare);
   return it != values.end() && it->first.is(name) ? it->second : Attribute();

diff --git a/mlir/lib/IR/MLIRContext.cpp b/mlir/lib/IR/MLIRContext.cpp
index 1623122..a3534bc 100644 (file)
--- a/mlir/lib/IR/MLIRContext.cpp
+++ b/mlir/lib/IR/MLIRContext.cpp
@@ -493,10 +493,6 @@ const AbstractOperation *AbstractOperation::lookup(StringRef opName,
 
 /// Return an identifier for the specified string.
 Identifier Identifier::get(StringRef str, MLIRContext *context) {
-  assert(!str.empty() && "Cannot create an empty identifier");
-  assert(str.find('\0') == StringRef::npos &&
-         "Cannot create an identifier with a nul character");
-
   auto &impl = context->getImpl();
 
   { // Check for an existing identifier in read-only mode.
@@ -506,6 +502,13 @@ Identifier Identifier::get(StringRef str, MLIRContext *context) {
       return Identifier(it->getKeyData());
   }
 
+  // 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, char()}).first;