[PATCH] D35702: [LTO][ThinLTO] Use the linker resolutions to mark global values as dso_local.

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 3 inline comments as done.
sfertile added inline comments.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:876
   bool Live = (RawFlags & 0x2) || Version < 3;
-  return GlobalValueSummary::GVFlags(Linkage, NotEligibleToImport, Live);
+  bool Local = (RawFlags & 0x4);
+
----------------
mehdi_amini wrote:
> inouehrs wrote:
> > Do we need some version check (and some comments) for compatibility with old bc files? 
> The default will be `0` for old .bc files, which translate to "not local", which should be conservatively OK I believe?
> 
> Ideally we would checking a bitcode file from the 5.0 branching point in the repo and test this behavior.
That was my reasoning for not needing a version check. 
I'll add the bitcode test from the 5.0  branch next update.


================
Comment at: lib/LTO/LTO.cpp:691
+        }
+      }
     }
----------------
mehdi_amini wrote:
> This does not seem safe in face of hash collision.
I've limited the summary look-up to the bitcode module we are currently adding, is this what you had in mind?



================
Comment at: lib/LTO/LTO.cpp:685
+        auto GUID = GlobalValue::getGUID(
+            GlobalValue::dropLLVMManglingEscape(Sym.getIRName()));
+        if (auto S = ThinLTO.CombinedIndex.findSummaryInModule(
----------------
I realize that this is the same GUID as calculated above since the call to getGlobalIdentifier is being passed  ExternalLinkage. I'll update this to reflect that.


Repository:
  rL LLVM

https://reviews.llvm.org/D35702