[PATCH] D18487: [ThinLTO] Add optional import message and statistics

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked an inline comment as done.
tejohnson added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp:359
+      for (const auto *GV : GlobalsToImport)
+        dbgs() << DestModule.getSourceFileName() << ": Import " << GV->getName()
+               << " from " << SrcModule->getSourceFileName() << "\n";
----------------
aganea wrote:
> Hello Teresa! This debug output is not thread-safe, but it is executed from ThinLTO threads. There's a test, `llvm/test/Transforms/PGOProfile/Inputs/thinlto_samplepgo_icp3.ll` which relies on this output. Currently that test only uses one thread (`-thinlto-threads=1`). However when using any other number of threads, the test randomly fails.
> I wanted to test other values for `-thinlto-threads` in D75153, but I cannot. I'd like to land that patch, and would like your advice.
> 
> What avenue would you suggest? Add/use a mutex here? Remove the coverage for `thinlto_samplepgo_icp3.ll` in D75153?
I don't think it is a problem unique to this particular debugging output. Any ThinLTO test that uses a -debug or -debug-only to get messages from backend threads will hit it as well if there is more than one thread.

I'd probably either remove the changes from that test, or better, use something other than the -print-inputs flag to test that the importing happened. I.e. check the llvm-dis on the -save-temps .import.bc file.

Actually,. that test looks like it has an issue - there is a line to test with the "; ICALL-PROM:" prefix, which is not ever actually invoked. Presumably could be fixed by also checking the llvm-dis of the temp file.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D18487