[PATCH] D121428: [lld-macho] Extend lto-internalize-unnamed-addr.ll

Tue Mar 15 17:23:37 PDT 2022

int3 marked an inline comment as done.
int3 added inline comments.

================
Comment at: lld/test/MachO/lto-internalize-unnamed-addr.ll:47
+; THINLTO-BC-DAG: @local_unnamed_sometimes_const = weak_odr local_unnamed_addr constant i8 42
+; THINLTO-BC-DAG: @local_unnamed = weak_odr local_unnamed_addr global i8 42
+
----------------
tejohnson wrote:
> int3 wrote:
> > tejohnson wrote:
> > > int3 wrote:
> > > > modimo wrote:
> > > > > I find it odd that `local_unnamed` is able to be internalized in fullLTO but not thinLTO. The code path is https://github.com/llvm/llvm-project/blob/9f616a467fc710f084b7e57812a2ed64c214c2c6/llvm/lib/LTO/LTO.cpp#L349 where any linkonce_odr gets upgraded to weak but if this is the only copy that exists it appears to fit within the rules coming from D20348. cc @tejohnson is there an opportunity here?
> > > > I realize `global_unnamed_sometimes_linkonce` has the same issue too. I've added a FIXME for this
> > > That's true, if there is only one copy no need to make the prevailing one weak_odr. Is changing that sufficient to get the internalization in the ThinLTO case?
> > I tried https://gist.github.com/int3/b66de20462da71e8757e09bacfc361ca but that just seems to result in `global_unnamed` and `local_unnamed_const` no longer being marked as `hidden`...
> To get hidden back this would need to be adjusted accordingly to handle linkonce_odr:
> http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#980
ah gotcha. yeah I got the `hidden`s back after changing that, as well as removing this earlier condition: http://llvm-cs.pcc.me.uk/lib/Transforms/IPO/FunctionImport.cpp#952

A bunch of `check-llvm` tests are now failing though. I would like to go through them at some point and figure out if they're legit, but I'd like to focus on other aspects of LLD for now. @modimo do you mind stamping this first?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121428