[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

In D97446#3241735 <https://reviews.llvm.org/D97446#3241735>, @MaskRay wrote:

> For your comment it appears an issue in the internalisation pass. It is orthogonal to this patch.
> Do you have a reduced example with reproduce instructions for the issue? I know very little about OpenMP.
> Well, I assume we can continuation the discussion in that OpenMP thread.

Internalize.cpp is fairly clear that it treats the two arrays differently, copying the corresponding part. Perhaps orthogonal to but exposed by this patch.

  // We must assume that globals in llvm.used have a reference that not even
  // the linker can see, so we don't internalize them.
  // For llvm.compiler.used the situation is a bit fuzzy. The assembler and
  // linker can drop those symbols. If this pass is running as part of LTO,
  // one might think that it could just drop llvm.compiler.used. The problem
  // is that even in LTO llvm doesn't see every reference. For example,
  // we don't see references from function local inline assembly. To be
  // conservative, we internalize symbols in llvm.compiler.used, but we
  // keep llvm.compiler.used so that the symbol is not deleted by llvm.
  for (GlobalValue *V : Used) {
    AlwaysPreserved.insert(V->getName());
  }



>> It's possible that the check in internalize that skips over llvm.used and not llvm.compiler.used is itself a bug, and the intent is for llvm.used to be identical to llvm.compiler.used, but if that's the case we should delete the llvm.compiler.used array.
>
> ...
> I agree that in many cases user don't need more fine-grained GC precision and probably just add both used/retain to not bother think about the problem.
> These people may find the split strange.
>
> llvm.compiler.used (the underlying mechanism of `__attribute__((used))`) is useful in instrumentations.
> There are quite few cases that the compiler does not fully discard definitions and has to defer it to the linker.
> I have changed some instrumentations (PGO/SanitizerCoverage/other sanitizers) to downgrade llvm.used to llvm.compiler.used to improve section based linker garbage collection for all of PE-COFF, Mach-O, and ELF.
> There has been decent object size decrease (at least single digit percent, 10+% for some).

Uh, yes. Discarding things that previously were not discarded will make things smaller. The users I'm advocating for here are the ones who would have preferred we not discard the things that they asked us to keep and that we used to keep. Perhaps they will be few in number, and at least there's a workaround to be discovered.

In D97446#3241767 <https://reviews.llvm.org/D97446#3241767>, @rjmccall wrote:

> I can understand how we got here, but it's a bad place to have ended up.
> ...elided...
> At the end of the day, I don't think we have much choice but to follow GCC's lead on ELF platforms.  They get to define what these attributes mean, and if they want to make weaker guarantees on ELF, that's their decision.

That's compelling, thank you for articulating it.

I think we have an outstanding issue to resolve in internalize, where the assumption behind this patch that the two forms are equivalent on elf does not hold.

I'd much refer we put variables in arrays corresponding to their intended lifespan instead of the binary format they're destined for. I don't know if the OS in question is certain to match the linker output either, seems possible to compile on an OS that usually uses one format but emit code in a different one.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2093
+         "Only globals with definition can force usage.");
+  if (getTriple().isOSBinFormatELF())
+    LLVMCompilerUsed.emplace_back(GV);
----------------
^ this specifically looks wrong, which array the variable goes in should be based on what the variable is used for or what the programmer asked for, not on the binary format used by the OS (is that even a unique test? One can run elf or coff on windows iirc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97446