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

[Reid Kleckner via Phabricator via llvm-commits]

rnk 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.
----------------
zequanwu wrote:
> 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.
Yes, sorry, my intent is to use Optional to handle the case where neither /OPT:REF nor /OPT:ICF appear. In that case, icfLevel will be None, and doICF should become ICFLevel::None. Maybe a better way of writing that is:
  if (!icfLevel)
    icfLevel = config->doGC ? ICFLevel::All : ICFLevel::None;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97436