[PATCH] D82193: [OpenMPOpt] ICV macro definitions

Thu Jul 2 14:03:42 PDT 2020

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

================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:230
+      ICV.InitValue =                                                          \
+          ConstantInt::get(Type::getInt32Ty(Int32->getContext()), 0);          \
+      break;                                                                   \
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82193