[PATCH] D20268: Resolution-based LTO API.

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 15 15:36:04 PDT 2016


mehdi_amini requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: include/llvm/LTO/LTO.h:94
@@ +93,3 @@
+class InputFile {
+  // FIXME: Remove LTO class friendship once we have bitcode symbol tables.
+  friend LTO;
----------------
> I agree with you, but until we have a symbol table, there needs to be some way for LTO to access IR entities in the input file.

It is not clear to me why LTO needs to access any IR entity? If you need access an *IR entity* now, how could we don't need access to them tomorrow (<- read: with a symbol table)?

It seems to me that anywhere you're accessing an IR entity directly through `Obj` right now is a place that should have an API on the InputFile or the Symbol class (like the one that exists: `InputFile::getSourceFileName()`).

Assuming "Symbol" is abstracting the entries in the symbol table, for instance:

 -  `Input->Obj->getMemoryBufferRef().getBufferIdentifier();` ->  `Input->getIdentifier()`
 -  `GlobalValue *GV = Obj->getSymbolGV(Sym.I->getRawDataRefImpl());` -> `Symbol *S = Obj->getSymbol(...)`
 - `GV->hasGlobalUnnamedAddr();` -> needs an API on class Symbol
 - `GV->getName();` -> needs an API on class Symbol
 - `if (Res.VisibleToRegularObj || (GV && Used.count(GV)) ||`  -> Used contains a `GlobalVariable *`, how do we go to a symbol table?
 - `Module &M = Input->Obj->getModule();` -> used for: 

```
  for (GlobalVariable &GV : M.globals())
    if (GV.hasAppendingLinkage())
      Keep.push_back(&GV);
```
which should iterate on `Symbol`s on the `InputFile`.

- `LTO::addRegularLto` does some symbol resolution using `Obj`. At this point you probably want to operate on real IR construct anyway, and that's the point where you leave the "InputFile" to go to the IR. I think `InputFile` should just expose something like a factory `std::unique_ptr<Module> parseModule(/* options */);`. If it was just for full LTO, that would solve many of the above items as `addSymbolToGlobalRes()` could take a `Module &` instead of operating on the private `IRObjectFile` inside `InputFile`. However it won't help for ThinLTO.

```
    GlobalValue *GV = Input->Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
    if (Res.Prevailing && GV)
      ThinLto.PrevailingModuleForGUID[GV->getGUID()] =
          MBRef.getBufferIdentifier();
```

- `getGUID()` should be implemented on the Symbol directly.

================
Comment at: include/llvm/LTO/LTO.h:103
@@ +102,3 @@
+  /// needed to supply an owning LLVMContext for the IRObjectFile.
+  static ErrorOr<std::unique_ptr<InputFile>> create(MemoryBufferRef Object,
+                                                    LTO &Lto);
----------------
I'm not sure, it seems easy to turn it around: "you are the one proposing an API that is targeted for performance reason with a specific use case (and client) in mind, so the burden should be on you to justify it if you want this to get it in.", otherwise write a cleaner API.

================
Comment at: lib/LTO/LTO.cpp:255
@@ +254,3 @@
+
+    GlobalValue *GV = Input->Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
+    if (Res.Prevailing && GV) {
----------------
Is is possible for GV to be null here?

================
Comment at: lib/LTO/LTO.cpp:280
@@ +279,3 @@
+Error LTO::addThinLto(std::unique_ptr<InputFile> Input,
+                      SmallPtrSet<GlobalValue *, 8> &Used,
+                      ArrayRef<SymbolResolution> Res) {
----------------
> Seems much simpler to always have a module with index 0.

"Simpler" from the gold point of view, my point of view is that when adding 1000 ThinLTO modules we'll copy 1000 DataLayout around for no reason.
Which reminds me that I don't expect any object being emitted for this module for a "pure" ThinLTO link.

================
Comment at: lib/LTO/LTO.cpp:305
@@ +304,3 @@
+
+    GlobalValue *GV = Input->Obj->getSymbolGV(Sym.I->getRawDataRefImpl());
+    if (Res.Prevailing && GV)
----------------
(Same question here, can GV be null?)


https://reviews.llvm.org/D20268





More information about the llvm-commits mailing list