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

[Zequan Wu via Phabricator via llvm-commits]

zequanwu added inline comments.


================
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.
----------------
rnk wrote:
> 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.
This will change the default behavior. Do you meant that? Or I misunderstand.
In current behavior, if  `doGC` is false and `/opt:icf` is not given (`icfLevel ==1`), it will set `icfLevel` to 0, which disable icf:
```
  if (icfLevel == 1 && !doGC)
    icfLevel = 0;
```
By your changes, if `doGC` is false  and `/opt:icf` is not given, `icfLevel` will be `ICFLevel::ALL` which enables icf.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97436