[PATCH] D103292: [lld-macho] Implement ICF

[Jez Ng via Phabricator via llvm-commits]

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

Neat! I think we still need a test for `N_ALT_ENTRY`, but I'm fine with having that in a follow-up (maybe just leave a TODO in icf.s).



================
Comment at: lld/MachO/Driver.cpp:693
 
+static ICFLevel getICFLevel(const ArgList &args) {
+  StringRef icfLevelStr = args.getLastArgValue(OPT_icf);
----------------
gkm wrote:
> int3 wrote:
> > should we have `-no_deduplicate` as a synonym for `-icf=none`?
> Are you proposing that `-no_deduplicate` be a strict synonym for `-icf=none`, or that it disable all forms of deduplication, including `--deduplicate_literals` ?
Good question. ld64 does literal dedup regardless of that flag, but it also runs ICF by default unless `-no_deduplicate` is passed. If we want to be a drop-in replacement we should match that behavior. But having only *some* optimizations run by default seems bizarre. It would probably be better to disable all optimizations by default and print out a message when `-no_deduplicate` is passed saying that it's now redundant. I guess we can sort that out in a future diff...


================
Comment at: lld/MachO/InputFiles.cpp:622-628
+      if (isZeroFill(isec->flags)) {
+        nextIsec->data = {nullptr, isec->data.size() - symbolOffset};
+        isec->data = {nullptr, symbolOffset};
+      } else {
+        nextIsec->data = isec->data.slice(symbolOffset);
+        isec->data = isec->data.slice(0, symbolOffset);
+      }
----------------
int3 wrote:
> gkm wrote:
> > int3 wrote:
> > > why is this change necessary?
> > Not strictly necessary. I saw SEGVs when checking `isec->data.data() == nullptr` because `parseSymbols` would advance the initial `nullptr` for a zero-fill section at every split. The alternative is to designate `data.data()` as undefined for zero-fill sections.
> oh that makes sense. Could you leave a comment to that effect?
I think you missed this one :)


================
Comment at: lld/test/MachO/icf.s:23-25
+# CHECK-LABEL: Disassembly of section __TEXT,__text:
+# CHECK: [[#%x,MAIN]] <_main>:
+# CHECK-NEXT: callq 0x[[#%x,F]]  <_f2>
----------------
nit: can we align the RHS of these lines?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103292