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

Wed Mar 25 15:44:08 PDT 2020

aganea 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";
----------------
tejohnson wrote:
> 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.
Just for the record, @tejohnson 's suggestion was implemented in rG934d4feab1f58ede295a3232ac47f3c01c9fc506.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D18487