[OpenMP] Ensure createXXX functions will always call updateToLocation

[jdoerfert]

Summary

It is easy to not set the debug location which will result in problems
in debug builds. Hence, we should always use the helper
updateToLocation for all user facing entry points of the OMPIRBuilder.

[kiranchandramohan]

LG. I have one question. Is there a contract in place where the InsertionPointGuard is used to reset the IP back to the one at entry and would this change break it?

[jdoerfert]

In D127399#3581355, @kiranchandramohan wrote:

LG. I have one question. Is there a contract in place where the InsertionPointGuard is used to reset the IP back to the one at entry and would this change break it?

I don't think the Builder should reset to entry. It's an implicit contract that is hard to maintain and arguably useless as there is no reason to believe one would insert two "OpenMP things" at exactly the same point. IPs during codegen generally move and we should make all IPs explicit. These guards were introduced in a single patch (IIRC) and should not serve a purpose.

[kiranchandramohan]

OK. LGTM.

[Meinersbur]

Why the pattern to just return without doing anything without an insert location? IMHO this should be an error instead of silently ignoring things.

In D127399#3583537, @jdoerfert wrote:

I don't think the Builder should reset to entry. It's an implicit contract that is hard to maintain and arguably useless as there is no reason to believe one would insert two "OpenMP things" at exactly the same point. IPs during codegen generally move and we should make all IPs explicit. These guards were introduced in a single patch (IIRC) and should not serve a purpose.

I completely agree and already encountered problems with the reset location having become invalid between that guard and its dtor.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

1846–1848

The insert- and debug location is already set inside createLoopSkeleton, setting it here has no effect.

On line 1849 updateToLocation is then called and it handles the "no insert location" case by not connecting the loop skeleton to anything (as the caller wishes). return null makes this impossible.

[jdoerfert]

In D127399#3623611, @Meinersbur wrote:

Why the pattern to just return without doing anything without an insert location? IMHO this should be an error instead of silently ignoring things.

Because that is the clang pattern and we want/need to be compatible.

In D127399#3583537, @jdoerfert wrote:

I don't think the Builder should reset to entry. It's an implicit contract that is hard to maintain and arguably useless as there is no reason to believe one would insert two "OpenMP things" at exactly the same point. IPs during codegen generally move and we should make all IPs explicit. These guards were introduced in a single patch (IIRC) and should not serve a purpose.

I completely agree and already encountered problems with the reset location having become invalid between that guard and its dtor.

[Meinersbur]

In D127399#3636310, @jdoerfert wrote:

Because that is the clang pattern and we want/need to be compatible.

Clang does not use such a pattern. When IRBuilder creates an instruction without an insert position, it still creates the instruction, but does not insert it into a function. createCanonicalLoop was supposed to do something similar (create the loop, but do not connect it to the CFG).

[jdoerfert]

In D127399#3636429, @Meinersbur wrote:

In D127399#3636310, @jdoerfert wrote:

Because that is the clang pattern and we want/need to be compatible.

Clang does not use such a pattern.

if (!CGF.HaveInsertPoint())                                                                                                                                                                                                                              
   return;

appears 35 times in CGOpenMPRuntime.cpp

[Meinersbur]

In D127399#3636469, @jdoerfert wrote:

if (!CGF.HaveInsertPoint())                                                                                                                                                                                                                              
   return;

appears 35 times in CGOpenMPRuntime.cpp

Commenting them all out still passes all tests. It essentially dead/untested code. Also seen in the code coverage: https://lab.llvm.org/coverage/coverage-reports/coverage/Users/buildslave/jenkins/workspace/coverage/llvm-project/clang/lib/CodeGen/CGOpenMPRuntime.cpp.html

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

 #endif
   return CL;
 }
 
 CanonicalLoopInfo *
 OpenMPIRBuilder::createCanonicalLoop(const LocationDescription &Loc,
                                      LoopBodyGenCallbackTy BodyGenCB,
                                      Value *TripCount, const Twine &Name) {
+  if (!updateToLocation(Loc))
+    return nullptr;
+
   BasicBlock *BB = Loc.IP.getBlock();
   BasicBlock *NextBB = BB->getNextNode();
 
   CanonicalLoopInfo *CL = createLoopSkeleton(Loc.DL, TripCount, BB->getParent(),
                                              NextBB, NextBB, Name);
   BasicBlock *After = CL->getAfter();
 
   // If location is not set, don't connect the loop.
CanonicalLoopInfo *OpenMPIRBuilder::createCanonicalLoop(
 
   // Start, Stop and Step must be of the same integer type.
   auto *IndVarTy = cast<IntegerType>(Start->getType());
   assert(IndVarTy == Stop->getType() && "Stop type mismatch");
   assert(IndVarTy == Step->getType() && "Step type mismatch");
 
   LocationDescription ComputeLoc =
       ComputeIP.isSet() ? LocationDescription(ComputeIP, Loc.DL) : Loc;
-  updateToLocation(ComputeLoc);
+  if (!updateToLocation(ComputeLoc))
+    return nullptr;
 
   ConstantInt *Zero = ConstantInt::get(IndVarTy, 0);
   ConstantInt *One = ConstantInt::get(IndVarTy, 1);
 
   // Like Step, but always positive.
   Value *Incr = Step;
 
   // Distance between Start and Stop; always positive.
OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::EmitOMPInlinedRegion(
   MergeBlockIntoPredecessor(FiniBB);
 
   // If we are skipping the region of a non conditional, remove the exit
   // block, and clear the builder's insertion point.
   assert(SplitPos->getParent() == ExitBB &&
          "Unexpected Insertion point location!");
   auto merged = MergeBlockIntoPredecessor(ExitBB);
   BasicBlock *ExitPredBB = SplitPos->getParent();
-  auto InsertBB = merged ? ExitPredBB : ExitBB;
+  auto *InsertBB = merged ? ExitPredBB : ExitBB;
   if (!isa_and_nonnull<BranchInst>(SplitPos))
     SplitPos->eraseFromParent();
   Builder.SetInsertPoint(InsertBB);
 
   return Builder.saveIP();
 }
 
 OpenMPIRBuilder::InsertPointTy OpenMPIRBuilder::emitCommonDirectiveEntry(
if (BranchtoEnd)
     Builder.SetInsertPoint(Builder.CreateBr(CopyEnd));
 
   return Builder.saveIP();
 }
 
 CallInst *OpenMPIRBuilder::createOMPAlloc(const LocationDescription &Loc,
                                           Value *Size, Value *Allocator,
                                           std::string Name) {
-  IRBuilder<>::InsertPointGuard IPG(Builder);
-  Builder.restoreIP(Loc.IP);
+  if (!updateToLocation(Loc))
+    return nullptr;
 
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
   Value *ThreadId = getOrCreateThreadID(Ident);
   Value *Args[] = {ThreadId, Size, Allocator};
 
   Function *Fn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_alloc);
 
   return Builder.CreateCall(Fn, Args, Name);
 }
 
 CallInst *OpenMPIRBuilder::createOMPFree(const LocationDescription &Loc,
                                          Value *Addr, Value *Allocator,
                                          std::string Name) {
-  IRBuilder<>::InsertPointGuard IPG(Builder);
-  Builder.restoreIP(Loc.IP);
+  if (!updateToLocation(Loc))
+    return nullptr;
 
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
   Value *ThreadId = getOrCreateThreadID(Ident);
   Value *Args[] = {ThreadId, Addr, Allocator};
   Function *Fn = getOrCreateRuntimeFunctionPtr(OMPRTL___kmpc_free);
   return Builder.CreateCall(Fn, Args, Name);
 }
 
 CallInst *OpenMPIRBuilder::createOMPInteropInit(
     const LocationDescription &Loc, Value *InteropVar,
     omp::OMPInteropType InteropType, Value *Device, Value *NumDependences,
     Value *DependenceAddress, bool HaveNowaitClause) {
-  IRBuilder<>::InsertPointGuard IPG(Builder);
-  Builder.restoreIP(Loc.IP);
+  if (!updateToLocation(Loc))
+    return nullptr;
 
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
   Value *ThreadId = getOrCreateThreadID(Ident);
   if (Device == nullptr)
     Device = ConstantInt::get(Int32, -1);
   Constant *InteropTypeVal = ConstantInt::get(Int64, (int)InteropType);
CallInst *OpenMPIRBuilder::createOMPInteropInit(
   Function *Fn = getOrCreateRuntimeFunctionPtr(OMPRTL___tgt_interop_init);
 
   return Builder.CreateCall(Fn, Args);
 }
 
 CallInst *OpenMPIRBuilder::createOMPInteropDestroy(
     const LocationDescription &Loc, Value *InteropVar, Value *Device,
     Value *NumDependences, Value *DependenceAddress, bool HaveNowaitClause) {
-  IRBuilder<>::InsertPointGuard IPG(Builder);
-  Builder.restoreIP(Loc.IP);
+  if (!updateToLocation(Loc))
+    return nullptr;
 
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
   Value *ThreadId = getOrCreateThreadID(Ident);
   if (Device == nullptr)
     Device = ConstantInt::get(Int32, -1);
   if (NumDependences == nullptr) {
CallInst *OpenMPIRBuilder::createOMPInteropDestroy(
   return Builder.CreateCall(Fn, Args);
 }
 
 CallInst *OpenMPIRBuilder::createOMPInteropUse(const LocationDescription &Loc,
                                                Value *InteropVar, Value *Device,
                                                Value *NumDependences,
                                                Value *DependenceAddress,
                                                bool HaveNowaitClause) {
-  IRBuilder<>::InsertPointGuard IPG(Builder);
-  Builder.restoreIP(Loc.IP);
+  if (!updateToLocation(Loc))
+    return nullptr;
+
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
   Value *ThreadId = getOrCreateThreadID(Ident);
   if (Device == nullptr)
     Device = ConstantInt::get(Int32, -1);
   if (NumDependences == nullptr) {
     NumDependences = ConstantInt::get(Int32, 0);
     PointerType *PointerTypeVar = Type::getInt8PtrTy(M.getContext());
     DependenceAddress = ConstantPointerNull::get(PointerTypeVar);
   }
   Value *HaveNowaitClauseVal = ConstantInt::get(Int32, HaveNowaitClause);
   Value *Args[] = {
       Ident,          ThreadId,          InteropVar,         Device,
       NumDependences, DependenceAddress, HaveNowaitClauseVal};
 
   Function *Fn = getOrCreateRuntimeFunctionPtr(OMPRTL___tgt_interop_use);
 
   return Builder.CreateCall(Fn, Args);
 }
 
 CallInst *OpenMPIRBuilder::createCachedThreadPrivate(
     const LocationDescription &Loc, llvm::Value *Pointer,
     llvm::ConstantInt *Size, const llvm::Twine &Name) {
-  IRBuilder<>::InsertPointGuard IPG(Builder);
-  Builder.restoreIP(Loc.IP);
+  if (!updateToLocation(Loc))
+    return nullptr;
 
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
   Value *ThreadId = getOrCreateThreadID(Ident);
   Constant *ThreadPrivateCache =
       getOrCreateOMPInternalVariable(Int8PtrPtr, Name);
   llvm::Value *Args[] = {Ident, ThreadId, Pointer, Size, ThreadPrivateCache};