[PATCH] D20268: Resolution-based LTO API.

[Mehdi AMINI via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

Last comments, we should be able to iterate in-tree after that.


================
Comment at: include/llvm/LTO/LTO.h:310
@@ +309,3 @@
+    IRMover Mover;
+  } RegularLto;
+
----------------
Should be `RegularLTO` I think.

================
Comment at: include/llvm/LTO/LTO.h:319
@@ +318,3 @@
+    DenseMap<GlobalValue::GUID, StringRef> PrevailingModuleForGUID;
+  } ThinLto;
+
----------------
Similarly, should be `ThinLTO`

================
Comment at: include/llvm/LTO/LTO.h:368
@@ +367,3 @@
+
+  Error addRegularLto(std::unique_ptr<InputFile> Input,
+                      ArrayRef<SymbolResolution> Res);
----------------
`addRegularLTO`

================
Comment at: include/llvm/LTO/LTO.h:370
@@ +369,3 @@
+                      ArrayRef<SymbolResolution> Res);
+  Error addThinLto(std::unique_ptr<InputFile> Input,
+                   ArrayRef<SymbolResolution> Res);
----------------
`addThinLTO`

================
Comment at: include/llvm/LTO/LTO.h:373
@@ +372,3 @@
+
+  Error runRegularLto(AddStreamFn AddStream);
+  Error runThinLto(AddStreamFn AddStream);
----------------
`runRegularLTO`

================
Comment at: include/llvm/LTO/LTO.h:374
@@ +373,3 @@
+  Error runRegularLto(AddStreamFn AddStream);
+  Error runThinLto(AddStreamFn AddStream);
+
----------------
`runThinLTO`

================
Comment at: lib/LTO/LTO.cpp:243
@@ +242,3 @@
+  MemoryBufferRef MBRef = Input->Obj->getMemoryBufferRef();
+  bool HasThinLtoSummary = hasGlobalValueSummary(MBRef, Conf.DiagHandler);
+
----------------
`HasThinLTOSummary`

================
Comment at: lib/LTO/LTO.cpp:281
@@ +280,3 @@
+    SymbolResolution Res = *ResI++;
+    addSymbolToGlobalRes(Obj.get(), Used, Sym, Res, 0);
+
----------------
I see, I was looking for an empty LTO module (`RegularLto.CombinedModule`) instead of object (as in produced object).



================
Comment at: lib/LTO/LTO.cpp:316
@@ +315,3 @@
+  // it as usual because the client may want to add symbol definitions to it.
+  if (M.getTargetTriple().empty()) {
+    RegularLto.CombinedModule->setTargetTriple(M.getTargetTriple());
----------------
Is this test what you intend? It seems reversed (if so, then it's probably not tested).

================
Comment at: lib/LTO/LTO.cpp:318
@@ +317,3 @@
+    RegularLto.CombinedModule->setTargetTriple(M.getTargetTriple());
+    RegularLto.CombinedModule->setDataLayout(M.getDataLayout());
+  }
----------------
This is still gonna copy a few SmallVector + a couple of string + a few other fields. And it's gonna happens for each and every ThinLTO module. This is just not the right place for that. Adding a triple/datalayout for LTO should not be handled here, it is just not the right place. `LTO::run` could handle it by poking at ThinLto.ModuleMap if needed.

Similarly, `RegularLto.CombinedModule` should not be created unless explicitly added/requested by the linker (i.e. not call to `make_unique<Module>("ld-temp.o", Ctx);` in the ctor).


================
Comment at: lib/LTO/LTO.cpp:358
@@ +357,3 @@
+    if (auto E = runRegularLto(AddStream))
+      return E;
+  return runThinLto(AddStream);
----------------
Doc the above tests.

================
Comment at: lib/LTO/LTOBackend.cpp:60
@@ +59,3 @@
+      } else
+        PathPrefix = M.getModuleIdentifier();
+      std::string Path = PathPrefix + "." + PathSuffix + ".bc";
----------------
It seems we are not looking at this from the same angle: I was looking at it as a *new* API, not a refactoring of whatever the gold plugin was doing. So I can't see "this is gold's behavior" as a valid motivation here (if you want this to be NFC from the gold point of view, the gold-plugin can be patched in the first place).




https://reviews.llvm.org/D20268