[PATCH] D20268: Resolution-based LTO API.

Teresa Johnson via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 10 17:39:54 PDT 2016


tejohnson added a comment.

pcc asked me to take over the patch since he is out on vacation this month. Responses below. I have made most of the suggested changes, but not a couple for the reasons stated. Let me see if I can upload the new patch even though pcc created this one.


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

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

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

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

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

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

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

================
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());
----------------
mehdi_amini wrote:
> Is this test what you intend? It seems reversed (if so, then it's probably not tested).
I think what was meant here was to check if the RegularLTO.CombinedModule's target triple was still empty, and if so set it. I have changed it to that.

================
Comment at: lib/LTO/LTO.cpp:318
@@ +317,3 @@
+    RegularLto.CombinedModule->setTargetTriple(M.getTargetTriple());
+    RegularLto.CombinedModule->setDataLayout(M.getDataLayout());
+  }
----------------
mehdi_amini wrote:
> 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).
> 
It only does the copy once with the change I made, so there isn't any duplication in the copying anymore. This is the simplest place to set it based on an added ThinLTO module.

I looked at delaying the creation of the CombinedModule, however, it turns out we frequently need it since even if we don't add any regular LTO files, we typically have callback hooks into the linker defined where the linker could add its own resolved symbols to the combined module, in which case it needs to be valid. I don't think we gain much by lazily creating the empty Module.

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

================
Comment at: lib/LTO/LTOBackend.cpp:60
@@ +59,3 @@
+      } else
+        PathPrefix = M.getModuleIdentifier();
+      std::string Path = PathPrefix + "." + PathSuffix + ".bc";
----------------
mehdi_amini wrote:
> 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).
> 
> 
I removed this path so that the OutputFileName is always used, so it now matches the doxygen comment for the API.


https://reviews.llvm.org/D20268





More information about the llvm-commits mailing list