[PATCH] D104671: [lld-macho] Extend ICF to literal sections

[Greg McGary via Phabricator via llvm-commits]

gkm accepted this revision.
gkm added a comment.
This revision is now accepted and ready to land.

Groovy!



================
Comment at: lld/MachO/ICF.cpp:112-113
             const auto *isecB = cast<ConcatInputSection>(db->isec);
-            if (isecA->icfEqClass[icfPass % 2] !=
-                isecB->icfEqClass[icfPass % 2])
-              return false;
-          } else {
-            // FIXME: implement ICF for other InputSection kinds
-            return false;
+            return da->value == db->value && isecA->icfEqClass[icfPass % 2] ==
+                                                 isecB->icfEqClass[icfPass % 2];
           }
----------------
I prefer this for convenient breakpoint locations when particular conditions fail.


================
Comment at: lld/MachO/ICF.cpp:117-119
+          return da->isec->parent == db->isec->parent &&
+                 da->isec->getOffset(da->value) ==
+                     db->isec->getOffset(db->value);
----------------
Ditto.


================
Comment at: lld/MachO/ICF.cpp:122
+        assert(da->isAbsolute() && db->isAbsolute());
+        return da->value != db->value;
       } else if (isa<DylibSymbol>(sa)) {
----------------
Ditto; and I think you meant this, no?
```
return da->value == db->value;
```


================
Comment at: lld/MachO/ICF.cpp:142
+               isa<WordLiteralInputSection>(sa));
+        return sa->getOffset(ra.addend) == sb->getOffset(rb.addend);
       }
----------------



================
Comment at: lld/MachO/ICF.cpp:211
+            if (defined->isec) {
+              if (auto isec = dyn_cast<ConcatInputSection>(defined->isec))
+                hash += isec->icfEqClass[icfPass % 2] + defined->value;
----------------



================
Comment at: lld/MachO/ICF.cpp:212
+              if (auto isec = dyn_cast<ConcatInputSection>(defined->isec))
+                hash += isec->icfEqClass[icfPass % 2] + defined->value;
+              else
----------------
Super nitty: I prefer to see the simpler oparand(s) come earlier in the expression.


================
Comment at: lld/test/MachO/icf-literals.s:32-35
+_bar1:
+  .asciz "foo"
+_bar2:
+  .asciz "foo"
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104671/new/

https://reviews.llvm.org/D104671