[PATCH] D97436: [lld-link] Fix addrsig symbols merging in ICF.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks, I have some more driver code simplification suggestions.



================
Comment at: lld/COFF/Driver.cpp:1555-1556
   bool doGC = debug == DebugKind::None || args.hasArg(OPT_profile);
   unsigned icfLevel =
-      args.hasArg(OPT_profile) ? 0 : 1; // 0: off, 1: limited, 2: on
+      args.hasArg(OPT_profile) ? 0 : 1; // 0: off, 1: limited, 2: on, 3: safe
   unsigned tailMerge = 1;
----------------
I see, I had forgotten about this limited ICF behavior. 


================
Comment at: lld/COFF/Driver.cpp:1608-1610
   // FIXME: LLD only implements "limited" ICF, i.e. it only merges identical
   // code. If the user passes /OPT:ICF explicitly, LLD should merge identical
   // comdat readonly data.
----------------
I think this FIXME is wrong, LLD will merge readonly data in both safe and regular ICF modes. In both modes, it will refuse to merge readonly data unless the object file has .llvm_addrsig and the data is not address taken.

Let's make the following changes:
- get rid of the "limited" ICF level 1, it is equivalent to ICFLevel::All
- Use Optional<ICFLevel> as the type for icfLevel
- `if (!icfLevel && config->doGC) icfLevel = ICFLevel::All;` -- that should make /opt:ref enable ICF
- Delete this FIXME comment and the discussion of limited ICF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97436