[AsmParser] Avoid instantiating LLVMContext if not needed. NFC

[yrouban]

Summary

Instantiating an LLVMContext might be uneasy task.

[jsilvanus]

llvm/lib/AsmParser/Parser.cpp

36

Instead of duplicating the call, maybe you can do something like

std::optional<LLVMContext> OptContext;
return LLParser(F.getBuffer(), SM, Err, M, Index,
                M ? M->getContext() : OptContext.emplace(), Slots)

[arsenm]

Can you reupload the patch with full context?

So this is recycling an existing, already used context? I keep running into bugs caused by doing that

[yrouban]

accepted the suggested snippet.

[yrouban]

I'm ready to land it as soon as it gets lgtm.

[jsilvanus]

The local change itself looks now good to me, but @arsenm may have general concerns with the approach here, so please address his comment first.

[yrouban]

I have added the diff with its full context.

[yrouban]

In D142699#4085204, @arsenm wrote:

Can you reupload the patch with full context?

Done

In D142699#4085204, @arsenm wrote:

So this is recycling an existing, already used context? I keep running into bugs caused by doing that

Can you elaborate? What bugs do you see? I'm currently investigating a crash during deletion of modules and functions after parsing, but my case does not use that local LLVMContext. So I just saw this extra instantiation and proposed to get rid of it.

[yrouban]

In D142699#4085204, @arsenm wrote:

So this is recycling an existing, already used context? I keep running into bugs caused by doing that

Please take a look at D143487. Could it be your case?

[yrouban]

The deleted copy constructor LLVMContext(LLVMContext &) got its
parameter changed to const to allow the latest clang compiler to
instantiatiate template std::optional<LLVMContext>.

[jsilvanus]

@arsenm are you OK with this now?

[yrouban]

@jsilvanus , @arsenm, please take a look.

In D142699#4212525, @jsilvanus wrote:

@arsenm are you OK with this now?

@jsilvanus, do you mind if I land the patch? I believe @arsenm just misunderstood the change. The given context is not recycled. The local context is recycled as it used to be. The only change is that the new local context is created only if it is really needed.

[jsilvanus]

LGTM.

This should be a strict improvement over the existing state, as we avoid unnecessary creations of contexts in some cases.

llvm/include/llvm/IR/LLVMContext.h

 /// (opaquely) owns and manages the core "global" data of LLVM's core
 /// infrastructure, including the type and constant uniquing tables.
 /// LLVMContext itself provides no locking guarantees, so you should be careful
 /// to have one context per thread.
 class LLVMContext {
 public:
   LLVMContextImpl *const pImpl;
   LLVMContext();
-  LLVMContext(LLVMContext &) = delete;
+  LLVMContext(const LLVMContext &) = delete;
   LLVMContext &operator=(const LLVMContext &) = delete;
   ~LLVMContext();
 
   // Pinned metadata names, which always have the same value.  This is a
   // compile-time performance optimization, not a correctness optimization.
   enum : unsigned {
 #define LLVM_FIXED_MD_KIND(EnumID, Name, Value) EnumID = Value,
 #include "llvm/IR/FixedMetadataKinds.def"

llvm/lib/AsmParser/Parser.cpp

 static bool parseAssemblyInto(MemoryBufferRef F, Module *M,
                               ModuleSummaryIndex *Index, SMDiagnostic &Err,
                               SlotMapping *Slots, bool UpgradeDebugInfo,
                               DataLayoutCallbackTy DataLayoutCallback) {
   SourceMgr SM;
   std::unique_ptr<MemoryBuffer> Buf = MemoryBuffer::getMemBuffer(F);
   SM.AddNewSourceBuffer(std::move(Buf), SMLoc());
 
-  LLVMContext Context;
+  std::optional<LLVMContext> OptContext;
   return LLParser(F.getBuffer(), SM, Err, M, Index,
-                  M ? M->getContext() : Context, Slots)
+                  M ? M->getContext() : OptContext.emplace(), Slots)
       .Run(UpgradeDebugInfo, DataLayoutCallback);
 }
 
 bool llvm::parseAssemblyInto(MemoryBufferRef F, Module *M,
                              ModuleSummaryIndex *Index, SMDiagnostic &Err,
                              SlotMapping *Slots,
                              DataLayoutCallbackTy DataLayoutCallback) {
   return ::parseAssemblyInto(F, M, Index, Err, Slots,
                              /*UpgradeDebugInfo*/ true, DataLayoutCallback);
 }