[PATCH] D71060: [LLD][ELF] Add time-trace to ELF LLD (2/2)

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Driver.cpp:505
 
+  // Initialize time trace.
+  if (config->timeTraceEnabled)
----------------
ruiu wrote:
> I'd move this above `initLLVM()` so that we can measure time consumed by createFiles and other functions.
> 
> Please remove `llvm::`.
s/time trace/time trace profiler/


================
Comment at: lld/ELF/Driver.cpp:505-509
+  // Initialize time trace.
+  if (config->timeTraceEnabled)
+    llvm::timeTraceProfilerInitialize(config->timeTraceGranularity,
+                                      config->progName);
+
----------------
I'd move this above `initLLVM()` so that we can measure time consumed by createFiles and other functions.

Please remove `llvm::`.


================
Comment at: lld/ELF/Driver.cpp:532
+
+  if (llvm::timeTraceProfilerEnabled()) {
+    SmallString<128> path(config->outputFile);
----------------
Let's use the same condition `if (config->timeTraceEnabled)` as before for consistency.


================
Comment at: lld/ELF/Driver.cpp:532
+
+  if (llvm::timeTraceProfilerEnabled()) {
+    SmallString<128> path(config->outputFile);
----------------
ruiu wrote:
> Let's use the same condition `if (config->timeTraceEnabled)` as before for consistency.
Add a single line comment -- // Write the result of the time trace profiler.


================
Comment at: lld/ELF/Driver.cpp:535-536
+    path.append(".time-trace");
+    if (!args.getLastArgValue(OPT_time_trace_file_eq).empty())
+      path = args.getLastArgValue(OPT_time_trace_file_eq);
+    std::error_code errorCode;
----------------
You could simplify this a little bit as shown below:

  std::string path = args.getLastArgValue(OPT_time_trace_file_eq);
  if (path.empty())
    path = (config->outputFile + ".time-trace").str();


================
Comment at: lld/ELF/Driver.cpp:537-538
+      path = args.getLastArgValue(OPT_time_trace_file_eq);
+    std::error_code errorCode;
+    raw_fd_ostream profilerOS(path, errorCode, sys::fs::F_Text);
+    if (errorCode) {
----------------
Let's use shorter conventional names: ec and os


================
Comment at: lld/ELF/Driver.cpp:544
+    llvm::timeTraceProfilerWrite(profilerOS);
+    llvm::timeTraceProfilerCleanup();
+  }
----------------
What does this cleanup function do? If some cleanup is needed, can we run it on timeTraceProfilerWrite?


================
Comment at: lld/ELF/ICF.cpp:530
 // ICF entry point function.
 template <class ELFT> void doIcf() { ICF<ELFT>().run(); }
 
----------------
This is the entry point function of ICF, so please move the TimeTraceScope here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71060/new/

https://reviews.llvm.org/D71060