[PATCH] D97436: [lld-link] Fix addrsig symbols merging in ICF.
Reid Kleckner via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 2 13:39:58 PST 2021
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
More information about the llvm-commits
mailing list