[OMPIRBuilder] Pass dependencies to createTask by value

[psoni2628]

Summary

This patch modifies OpenMPIRBuilder::createTask to accept its
Dependencies vector by value instead of by reference. This is
necessary because the PostOutlineCB lambda that uses this Dependencies
vector may outlive the original Dependencies vector.

[kiranchandramohan]

Assuming this was not triggered because we never used Dependencies vector before and no other PostOutlineCB took an option (with a reference) like this.

[shraiysh]

Please add a test case which would fail with the old implementation but passes with the new one.

[psoni2628]

In D141651#4058516, @kiranchandramohan wrote:

Assuming this was not triggered because we never used Dependencies vector before and no other PostOutlineCB took an option (with a reference) like this.

Yes that's right. There is no other PostOutlineCB that has a SmallVector being passed as an ArrayRef.

In D141651#4059097, @shraiysh wrote:

Please add a test case which would fail with the old implementation but passes with the new one.

I have adjusted the existing test case slightly to fail with the old implementation. There isn't really any point of keeping the old test case around because it is unrealistic. The original stack-allocated DependData should not be expected to still be in scope when the PostOutlineCB lambda is called.

[psoni2628]

• Modify test case slightly to fail with the old logic

[kiranchandramohan]

LG.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

public:
   ///                    suspend the current task region, for which execution
   ///                    cannot be resumed until execution of the structured
   ///                    block that is associated with the generated task is
   ///                    completed.
   InsertPointTy createTask(const LocationDescription &Loc,
                            InsertPointTy AllocaIP, BodyGenCallbackTy BodyGenCB,
                            bool Tied = true, Value *Final = nullptr,
                            Value *IfCondition = nullptr,
-                           ArrayRef<DependData *> Dependencies = {});
+                           SmallVector<DependData> Dependencies = {});
 
   /// Generator for the taskgroup construct
   ///
   /// \param Loc The location where the taskgroup construct was encountered.
   /// \param AllocaIP The insertion point to be used for alloca instructions.
   /// \param BodyGenCB Callback that will generate the region code.
   InsertPointTy createTaskgroup(const LocationDescription &Loc,
                                 InsertPointTy AllocaIP,

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

if (!updateToLocation(Loc))
     return;
   emitTaskyieldImpl(Loc);
 }
 
 OpenMPIRBuilder::InsertPointTy
 OpenMPIRBuilder::createTask(const LocationDescription &Loc,
                             InsertPointTy AllocaIP, BodyGenCallbackTy BodyGenCB,
                             bool Tied, Value *Final, Value *IfCondition,
-                            ArrayRef<DependData *> Dependencies) {
+                            SmallVector<DependData> Dependencies) {
   if (!updateToLocation(Loc))
     return InsertPointTy();
 
   uint32_t SrcLocStrSize;
   Constant *SrcLocStr = getOrCreateSrcLocStr(Loc, SrcLocStrSize);
   Value *Ident = getOrCreateIdent(SrcLocStr, SrcLocStrSize);
   // The current basic block is split into four basic blocks. After outlining,
   // they will be mapped as follows:
if (Dependencies.size()) {
       Builder.SetInsertPoint(
           &OldIP.getBlock()->getParent()->getEntryBlock().back());
 
       Type *DepArrayTy = ArrayType::get(DependInfo, Dependencies.size());
       Value *DepArray =
           Builder.CreateAlloca(DepArrayTy, nullptr, ".dep.arr.addr");
 
       unsigned P = 0;
-      for (DependData *Dep : Dependencies) {
+      for (const DependData &Dep : Dependencies) {
         Value *Base =
             Builder.CreateConstInBoundsGEP2_64(DepArrayTy, DepArray, 0, P);
         // Store the pointer to the variable
         Value *Addr = Builder.CreateStructGEP(
             DependInfo, Base,
             static_cast<unsigned int>(RTLDependInfoFields::BaseAddr));
         Value *DepValPtr =
-            Builder.CreatePtrToInt(Dep->DepVal, Builder.getInt64Ty());
+            Builder.CreatePtrToInt(Dep.DepVal, Builder.getInt64Ty());
         Builder.CreateStore(DepValPtr, Addr);
         // Store the size of the variable
         Value *Size = Builder.CreateStructGEP(
             DependInfo, Base,
             static_cast<unsigned int>(RTLDependInfoFields::Len));
         Builder.CreateStore(Builder.getInt64(M.getDataLayout().getTypeStoreSize(
-                                Dep->DepValueType)),
+                                Dep.DepValueType)),
                             Size);
         // Store the dependency kind
         Value *Flags = Builder.CreateStructGEP(
             DependInfo, Base,
             static_cast<unsigned int>(RTLDependInfoFields::Flags));
         Builder.CreateStore(
             ConstantInt::get(Builder.getInt8Ty(),
-                             static_cast<unsigned int>(Dep->DepKind)),
+                             static_cast<unsigned int>(Dep.DepKind)),
             Flags);
         ++P;
       }
 
       DepArrayPtr = Builder.CreateBitCast(DepArray, Builder.getInt8PtrTy());
       Builder.restoreIP(OldIP);
     }
 

llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp

TEST_F(OpenMPIRBuilderTest, CreateTaskDepend) {
   F->setName("func");
   IRBuilder<> Builder(BB);
   auto BodyGenCB = [&](InsertPointTy AllocaIP, InsertPointTy CodeGenIP) {};
   BasicBlock *AllocaBB = Builder.GetInsertBlock();
   BasicBlock *BodyBB = splitBB(Builder, /*CreateBranch=*/true, "alloca.split");
   OpenMPIRBuilder::LocationDescription Loc(
       InsertPointTy(BodyBB, BodyBB->getFirstInsertionPt()), DL);
   AllocaInst *InDep = Builder.CreateAlloca(Type::getInt32Ty(M->getContext()));
+  SmallVector<OpenMPIRBuilder::DependData> DDS;
+  {
     OpenMPIRBuilder::DependData DDIn(RTLDependenceKindTy::DepIn,
                                      Type::getInt32Ty(M->getContext()), InDep);
-  SmallVector<OpenMPIRBuilder::DependData *, 4> DDS;
-  DDS.push_back(&DDIn);
+    DDS.push_back(DDIn);
+  }
   Builder.restoreIP(OMPBuilder.createTask(
       Loc, InsertPointTy(AllocaBB, AllocaBB->getFirstInsertionPt()), BodyGenCB,
       /*Tied=*/false, /*Final*/ nullptr, /*IfCondition*/ nullptr, DDS));
   OMPBuilder.finalize();
   Builder.CreateRetVoid();
 
   // Check for the `NumDeps` argument
   CallInst *TaskAllocCall = dyn_cast<CallInst>(