[PATCH] D82193: [OpenMPOpt] ICV macro definitions

Thu Jul 2 15:08:27 PDT 2020

hoyFB added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:230
+      ICV.InitValue =                                                          \
+          ConstantInt::get(Type::getInt32Ty(Int32->getContext()), 0);          \
+      break;                                                                   \
----------------
jdoerfert wrote:
> sstefan1 wrote:
> > hoyFB wrote:
> > > christylee wrote:
> > > > sstefan1 wrote:
> > > > > jdoerfert wrote:
> > > > > > sstefan1 wrote:
> > > > > > > hoyFB wrote:
> > > > > > > > Hello,
> > > > > > > > 
> > > > > > > > Looks like the `Int32` type with the full name `llvm::omp::types::Int32` is a global variable. Creating a value using it may cause a race condition in thinLTO mode.  Although `Int32` is initialized with a module specific context, but it may or never get re-created per module. Please correct me if I misunderstand anything. Thanks.
> > > > > > > > 
> > > > > > > > Below is the my failing call stack in thinLTO mode:
> > > > > > > > 
> > > > > > > > PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace.
> > > > > > > > Stack dump:
> > > > > > > > 0.	Running pass 'CallGraph Pass Manager' on module ...'.
> > > > > > > >  #0 0x00000000014a3a8f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:564:13
> > > > > > > >  #1 0x00000000014a1c30 llvm::sys::RunSignalHandlers() /home/hoy/local/server-llvm/llvm-project/llvm/lib/Support/Signals.cpp:69:18
> > > > > > > >  #2 0x00000000014a402c SignalHandler(int) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:396:3
> > > > > > > >  #3 0x00007f00baf0b630 __restore_rt (/lib64/libpthread.so.0+0xf630)
> > > > > > > >  #4 0x00000000030afd08 llvm::ConstantInt::get(llvm::LLVMContext&, llvm::APInt const&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/IR/Constants.cpp:776:40
> > > > > > > >  #5 0x00000000030b2a9b llvm::ConstantInt::get(llvm::IntegerType*, unsigned long, bool) /home/hoy/local/server-llvm/llvm-project/llvm/lib/IR/Constants.cpp:797:10
> > > > > > > >  #6 0x00000000024ccc56 (anonymous namespace)::OMPInformationCache::initializeInternalControlVars() /home/hoy/local/server-llvm/llvm-project/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:326:1
> > > > > > > >  #7 0x00000000024ccc56 (anonymous namespace)::OMPInformationCache::OMPInformationCache(llvm::Module&, llvm::AnalysisGetter&, llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>&, llvm::SetVector<llvm::Function*, std::vector<llvm::Function*, std::allocator<llvm::Function*> >, llvm::DenseSet<llvm::Function*, llvm::DenseMapInfo<llvm::Function*> > >*, llvm::SmallPtrSetImpl<llvm::Function*>&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:69:0
> > > > > > > >  #8 0x00000000024f10ff llvm::SmallVectorTemplateCommon<llvm::Function*, void>::begin() /home/hoy/local/server-llvm/llvm-project/llvm/include/llvm/ADT/SmallVector.h:152:45
> > > > > > > >  #9 0x00000000024f10ff (anonymous namespace)::OpenMPOpt::OpenMPOpt(llvm::SmallVectorImpl<llvm::Function*>&, llvm::CallGraphUpdater&, llvm::function_ref<llvm::OptimizationRemarkEmitter& (llvm::Function*)>, (anonymous namespace)::OMPInformationCache&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:336:0
> > > > > > > > #10 0x00000000024f10ff (anonymous namespace)::OpenMPOptLegacyPass::runOnSCC(llvm::CallGraphSCC&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:834:0
> > > > > > > Not really sure, but turns out types were initialized twice, maybe that was the problem. I guess I could also get module context explicitly , instead of using `Int32->getContext()`. 
> > > > > > > 
> > > > > > > Anyway, I'll try to build with thinLTO and report back.
> > > > > > > Not really sure, but turns out types were initialized twice
> > > > > > 
> > > > > > That is a problem. Can you fix that first. I would not be surprised if the LTO error goes away.
> > > > > @hoyFB  I wasn't able to reproduce this locally, but I committed a potential fix: rG61238d2690a6ebdf3c4f3f68f39101fac30263a7
> > > > > 
> > > > > Can you check again if the problem is still there?
> > > > @sstefan1 Thanks!  I'll check and report back.
> > > @sstefan1 Thanks for the quick response. I applied the fix and the problem is still there:
> > > 
> > > 
> > > ```
> > > #0 0x0000000001474c2f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:564:13
> > >  #1 0x0000000001472dd0 llvm::sys::RunSignalHandlers() /home/hoy/local/server-llvm/llvm-project/llvm/lib/Support/Signals.cpp:69:18
> > >  #2 0x00000000014751cc SignalHandler(int) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Support/Unix/Signals.inc:396:3
> > >  #3 0x00007fb6ac7cc630 __restore_rt (/lib64/libpthread.so.0+0xf630)
> > >  #4 0x0000000003107065 llvm::Type::getInt32Ty(llvm::LLVMContext&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/IR/Type.cpp:186:66
> > >  #5 0x0000000002462754 (anonymous namespace)::OMPInformationCache::initializeInternalControlVars() /home/hoy/local/server-llvm/llvm-project/llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:326:1
> > >  #6 0x0000000002462754 (anonymous namespace)::OMPInformationCache::OMPInformationCache(llvm::Module&, llvm::AnalysisGetter&, llvm::BumpPtrAllocatorImpl<llvm::MallocAllocator, 4096ul, 4096ul, 128ul>&, llvm::SetVector<llvm::Function*, std::vector<llvm::Function*, std::allocator<llvm::Function*> >, llvm::DenseSet<llvm::Function*, llvm::DenseMapInfo<llvm::Function*> > >*, llvm::SmallPtrSetImpl<llvm::Function*>&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:69:5
> > >  #7 0x0000000002484e0f llvm::SmallVectorTemplateCommon<llvm::Function*, void>::begin() /home/hoy/local/server-llvm/llvm-project/llvm/include/llvm/ADT/SmallVector.h:152:45
> > >  #8 0x0000000002484e0f (anonymous namespace)::OpenMPOpt::OpenMPOpt(llvm::SmallVectorImpl<llvm::Function*>&, llvm::CallGraphUpdater&, llvm::function_ref<llvm::OptimizationRemarkEmitter& (llvm::Function*)>, (anonymous namespace)::OMPInformationCache&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:334:18
> > >  #9 0x0000000002484e0f (anonymous namespace)::OpenMPOptLegacyPass::runOnSCC(llvm::CallGraphSCC&) /home/hoy/local/server-llvm/llvm-project/llvm/lib/Transforms/IPO/OpenMPOpt.cpp:832:15
> > > ```
> > > 
> > > It seems we still have a race condition. The `Int32` type can be initialized by a module and then consumed by another module simultaneously. Your early proposal to use the explicit module context to create a constant value instead of using `Int32.getContext()` which can be from whatever module that first initializes the type sounds good to me. Thanks. 
> > > 
> > Can you provide some details on how to reproduce this? I wasn't able to. I would like to be able to test this locally.
> I think I understand now. Two processes spin up OMPIRBuilder or OpenMPOpt passes at the same time, both of those initialize global types, we created a race on these globals and all bets are off. I assume (thin)LTO has a single `llvm::Context` so we "just" need to prevent two threads from initializing at the same time. LLVM has a RAII mutex in `llvm/include/llvm/Support/Mutex.h` (`SmartScopedLock`) that we should place in a (new) initializer function (which calls the initializer functions we call now in the constructor) to prevent the race.
The issue happened to one of our internal large projects with many files having OpenMP usage. The project is built with 40 threads in thinLTO mode. I guess it doesn't reproduce with smaller projects. 

You may want to print out the `Int32.getContext()` object as well as the `Module` object for each  `ConstantInt::get(Type::getInt32Ty(Int32->getContext()), 0);` call and see if they match. Two modules creating constants in the same context may cause a problem.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82193