[PATCH] D82193: [OpenMPOpt] ICV macro definitions

Thu Jul 2 15:08:29 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;                                                                   \
----------------
hoyFB wrote:
> 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.
Yeah, that's the problem. 

The global OMP types are initialized without lock protection with a module local context:

https://github.com/llvm/llvm-project/blob/1a70077b5a64189d9c04d1a2d7ea6ff0e49744d6/llvm/lib/Frontend/OpenMP/OMPConstants.cpp#L73

Even if they are lock protected, the module local context passed to them can still be used to create a constant by another module in parallel. Module constants should be created in sequence.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82193