[lld] [lld][MachO] Key branch-extension thunks on (referent, addend) (PR #191808)

[Ellis Hoag via llvm-commits]

================
@@ -112,32 +112,60 @@ NamePair maybeRenameSection(NamePair key);
 // of ConcatOutputSection, so must have deterministic iteration order.
 extern llvm::MapVector<NamePair, ConcatOutputSection *> concatOutputSections;
 
-// After ICF, multiple Defined symbols may point to the same (isec, value) yet
-// remain as distinct Symbol pointers.  Using bare Symbol* as thunkMap keys
-// would create redundant thunks for the same target. This DenseMapInfo
+// Branch-extension thunks are keyed by both the target referent and the
+// branch relocation's addend.  Two call sites that branch to the same
+// symbol with different addends (e.g. `bl _func` and `bl _func+8`) target
+// distinct addresses and therefore need distinct thunks.
+//
+// After ICF, multiple Defined symbols may point to the same (isec, value)
+// yet remain as distinct Symbol pointers.  The equality predicate below
 // canonicalizes Defined symbols by (isec, value) so that ICF-folded copies
-// share a single thunkMap entry.
-struct ThunkMapKeyInfo : llvm::DenseMapInfo<Symbol *> {
-  static unsigned getHashValue(const Symbol *sym) {
-    if (const auto *d = dyn_cast<Defined>(sym))
-      return llvm::hash_combine(d->isec(), d->value);
-    return llvm::DenseMapInfo<Symbol *>::getHashValue(sym);
+// still share a single thunkMap entry when their addends match.
+struct ThunkKey {
+  Symbol *sym;
+  int64_t addend;
+
+  static ThunkKey getEmptyKey() {
+    return {llvm::DenseMapInfo<Symbol *>::getEmptyKey(), 0};
+  }
+  static ThunkKey getTombstoneKey() {
+    return {llvm::DenseMapInfo<Symbol *>::getTombstoneKey(), 0};
   }
-  static bool isEqual(const Symbol *lhs, const Symbol *rhs) {
-    if (lhs == rhs)
+  bool isSentinel() const {
+    return sym == llvm::DenseMapInfo<Symbol *>::getEmptyKey() ||
+           sym == llvm::DenseMapInfo<Symbol *>::getTombstoneKey();
+  }
+  bool operator==(const ThunkKey &other) const {
+    if (addend != other.addend)
+      return false;
+    if (sym == other.sym)
       return true;
-    if (lhs == getEmptyKey() || lhs == getTombstoneKey() ||
-        rhs == getEmptyKey() || rhs == getTombstoneKey())
+    if (isSentinel() || other.isSentinel())
       return false;
-    const auto *dl = dyn_cast<Defined>(lhs);
-    const auto *dr = dyn_cast<Defined>(rhs);
+    const auto *dl = dyn_cast<Defined>(sym);
+    const auto *dr = dyn_cast<Defined>(other.sym);
     if (dl && dr)
       return dl->isec() == dr->isec() && dl->value == dr->value;
     return false;
   }
 };
 
-extern llvm::DenseMap<Symbol *, ThunkInfo, ThunkMapKeyInfo> thunkMap;
+struct ThunkMapKeyInfo {
+  static ThunkKey getEmptyKey() { return ThunkKey::getEmptyKey(); }
+  static ThunkKey getTombstoneKey() { return ThunkKey::getTombstoneKey(); }
+  static unsigned getHashValue(const ThunkKey &k) {
+    if (k.isSentinel())
+      return llvm::hash_value(k.sym);
+    if (const auto *d = dyn_cast<Defined>(k.sym))
+      return llvm::hash_combine(d->isec(), d->value, k.addend);
+    return llvm::hash_combine(k.sym, k.addend);
+  }
+  static bool isEqual(const ThunkKey &lhs, const ThunkKey &rhs) {
+    return lhs == rhs;
+  }
+};
----------------
ellishg wrote:

I'm not sure, but if we implement `getHashValue()` for `ThunkKey` will we get this info struct for free? Can we then remove `ThunkMapKeyInfo` from the definition below?

https://github.com/llvm/llvm-project/pull/191808