[PATCH] D26832: [LTO] Add option to generate optimization records

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: lib/LTO/LTOCodeGenerator.cpp:96-100
+static cl::opt<std::string>
+    RemarksFilename("pass-remarks-output",
+                    cl::desc("Output filename for pass remarks"),
+                    cl::value_desc("filename"));
+
----------------
davide wrote:
> anemet wrote:
> > davide wrote:
> > > Side note, I very much dislike adding other `cl::opt` in library code, but this is what we have now. If I recall correctly, @beanz has plans for moving away of them, but not sure how far away it is.
> > > If you use the new `lib/LTO` interface maybe you will be able to get rid of it (but you should use `llvm-lto2` to test.
> > Does the Darwin linker support the new interface?
> I don't know about ld64 (and I don't think you're talking about lld as it doesn't do LTO on Mach-O).
> So, unfortunately, it seems that `cl::opt` is the only viable option.
In general it is not OK to use cl::opt for "user interface" or "api option" in library.
However I don't believe there is any other option here, and as long as it is contained in the LTOCodeGenerator that's fine: cl::opt is the way to initialize it on Darwin.
That said I think we should contain them in `lto.cpp` and expose APIs in LTOCodeGenerator.
We added two in LTOCodegenerator already, but that wasn't the right thing to do.


================
Comment at: lib/LTO/LTOCodeGenerator.cpp:519
+
+void LTOCodeGenerator::finishOptimiationRemarks() {
+  if (DiagnosticOutputFile) {
----------------
`s/Optimiation/Optimization/`


================
Comment at: lib/LTO/LTOCodeGenerator.cpp:521-523
+    DiagnosticOutputFile->keep();
+    // FIXME: LTOCodeGenerator dtor is not invoked on Darwin
+    DiagnosticOutputFile->os().flush();
----------------
davide wrote:
> anemet wrote:
> > davide wrote:
> > > have you already diagnosed why?
> > I *think* that's a known bug in our linker.  Mehdi can correct me if I am wrong.
> Fair enough.
This is similarly to clang that does not destroy the context in production, the Darwin linker does not clear any state before exiting.


https://reviews.llvm.org/D26832