[OpenMP 5.0] Fix user-defined mapper privatization in tasks

[ABataev]

Summary

This patch fixes the problem that user-defined mapper array is not correctly privatized inside a task. This problem causes openmp/libomptarget/test/offloading/target_depend_nowait.cpp fails.

[grokos]

I tried the patch and indeed it fixes the problem with target_depend_nowait.cpp. I'll let someone else do the review though.

[protze.joachim]

I'm not enough invested in the compiler implementation to judge this patch.
I can confirm that this patch fixes the segmentation fault I have seen.

Are these tests supposed to work for offloading to nvidia GPU as well?
I tried and added:

// RUN: %libomptarget-compilexx-run-and-check-nvptx64-nvidia-cuda

which gives me:

ptxas /tmp/target_depend_nowait-37c5c4.s, line 355; error   : Call has wrong number of parameters
ptxas /tmp/target_depend_nowait-37c5c4.s, line 628; error   : Call has wrong number of parameters
ptxas /tmp/target_depend_nowait-37c5c4.s, line 2331; warning : Instruction 'vote' without '.sync' may produce unpredictable results on sm_70 and later architectures
ptxas /tmp/target_depend_nowait-37c5c4.s, line 2331; warning : Instruction 'vote' without '.sync' is deprecated since PTX ISA version 6.0 and will be discontinued in a future PTX ISA version
ptxas fatal   : Ptx assembly aborted due to errors

I guess this error is not directly related to your patch?

clang/lib/CodeGen/CGOpenMPRuntime.cpp

10445

Does this mean, that a task is only created if depend clauses are provided?
Does waiting with taskwait work without this wrapper task?

[lildmh]

The problem you had is probably not related to this patch. A lot of libomptarget tests are not working for nvptx now. I suppose someone is going to fix them.

clang/lib/CodeGen/CGOpenMPRuntime.cpp

10445

The case you describe, which is to have a task wrapped around target, is solved with modification on line 9592
This part is to deal with other cases, which is consistent with line 10455 if you look a bit further

[ABataev]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

8840

Do you really need this IsTask flag? The array is created always when Info.NumberOfPtrs > 0. Maybe, just do not check for HasMapper here?

10445

Better to add a flag RequiresOuterTask = D.hasClausesOfKind<OMPDependClause>() and use it here and where the task is generated (line 10455)

[lildmh]

clang/lib/CodeGen/CGOpenMPRuntime.cpp

8840

This is an optimization. The mapper array can be optimized out when there is no actual mappers, so supposed to have less memory accesses in this common case. Not checking HasMapper will disable this optimization.

10445

Sure, please accept this patch after that.

[ABataev]

Add a test

[ye-luo]

What is the current status of this patch?
@lildmh could you update this patch? I'd like to test it against
https://bugs.llvm.org/show_bug.cgi?id=47122

[protze.joachim]

In D84470#2191752, @ABataev wrote:

Add a test

@ABataev did you ask for a codegen test, or is the runtime test openmp/libomptarget/test/offloading/target_depend_nowait.cpp enough?
The test fails without this patch and succeeds with this patch.

[ABataev]

In D84470#2223678, @protze.joachim wrote:

In D84470#2191752, @ABataev wrote:

Add a test

@ABataev did you ask for a codegen test, or is the runtime test openmp/libomptarget/test/offloading/target_depend_nowait.cpp enough?
The test fails without this patch and succeeds with this patch.

Yes, I know that it fails, but each functional change in the frontend must have the corresponding test in clang itself.

[lildmh]

I'll add a test to it. Please give me some time as I am busy with other stuffs.

[jdoerfert]

It looks like @lildmh is not able to provide a test any time soon. @grokos Could you take over this mapper patch as well? I think the current segfault state is rather unfortunate.

[ronlieb]

i applied this patch to our downstream build for amdgcn. it resolves the problem i was seeing with traps on tests with task wait depend tests from sollve.
perhaps rebase it to latest llvm.

[ABataev]

I can update this patch in a week when I'm back from vacation.

[ronlieb]

[AMD Official Use Only - Internal Distribution Only]

Hi Alexey
Any ETA on this one ?

Get Outlook for iOShttps://aka.ms/o0ukef

---

From: Alexey Bataev via Phabricator <[e-mail]>
Sent: Tuesday, September 8, 2020 1:39:06 PM
To: [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Cc: Lieberman, Ron <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; Liu, Yaxun (Sam) <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Subject: [PATCH] D84470: [OpenMP 5.0] Fix user-defined mapper privatization in tasks

[CAUTION: External Email]

ABataev added a comment.

I can update this patch in a week when I'm back from vacation.

CHANGES SINCE LAST ACTION

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD84470%2Fnew%2F&data=02%7C01%7Cron.lieberman%40amd.com%7C6582a2f2d31d4bbf9b4a08d8541e16cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637351835496784231&sdata=8%2BJbFULmzVN%2FdafC0QYX%2B9LwN7d1HFZ9Qxt6k4y85%2FA%3D&reserved=0

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freviews.llvm.org%2FD84470&amp;data=02%7C01%7Cron.lieberman%40amd.com%7C6582a2f2d31d4bbf9b4a08d8541e16cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637351835496794187&amp;sdata=gyj3%2B4K3oPlvedNjOzbsnosciYUIxuTOoKq%2BsfnsAOQ%3D&amp;reserved=0

[ABataev]

Hi Ron, hope to prepare it by the end of this week, maybe earlier, if everything goes well. I have some technical issues, but anyway will update the patch and the tests.

[ABataev]

Updated + added clang test.

[ronlieb]

The latest patch applied cleanly to our downstream port.
builds fine, tests very nicely as well. All the failing SOLLVE task wait depend tests now pass.

[ABataev]

In D84470#2277991, @ronlieb wrote:

The latest patch applied cleanly to our downstream port.
builds fine, tests very nicely as well. All the failing SOLLVE task wait depend tests now pass.

Could you accept it then?

[ronlieb]

accepted. not sure if there were any outstanding comments from Johannes. your call

clang/lib/CodeGen/CGOpenMPRuntime.cpp

OpenMPDirectiveKind Kind = isOpenMPTaskLoopDirective(D.getDirectiveKind())
                                  ? OMPD_taskloop
                                  : OMPD_task;
   const CapturedStmt &CS = *D.getCapturedStmt(Kind);
   CodeGenFunction::CGCapturedStmtInfo CapturesInfo(CS);
   LValue SrcBase;
   bool IsTargetTask =
       isOpenMPTargetDataManagementDirective(D.getDirectiveKind()) ||
       isOpenMPTargetExecutionDirective(D.getDirectiveKind());
-  // For target-based directives skip 3 firstprivate arrays BasePointersArray,
-  // PointersArray and SizesArray. The original variables for these arrays are
-  // not captured and we get their addresses explicitly.
+  // For target-based directives skip 4 firstprivate arrays BasePointersArray,
+  // PointersArray, SizesArray, and MappersArray. The original variables for
+  // these arrays are not captured and we get their addresses explicitly.
   if ((!IsTargetTask && !Data.FirstprivateVars.empty() && ForDup) ||
       (IsTargetTask && KmpTaskSharedsPtr.isValid())) {
     SrcBase = CGF.MakeAddrLValue(
         CGF.Builder.CreatePointerBitCastOrAddrSpaceCast(
             KmpTaskSharedsPtr, CGF.ConvertTypeForMem(SharedsPtrTy)),
         SharedsTy);
   }
   FI = cast<RecordDecl>(FI->getType()->getAsTagDecl())->field_begin();
   for (const PrivateDataTy &Pair : Privates) {
     const VarDecl *VD = Pair.second.PrivateCopy;
     const Expr *Init = VD->getAnyInitializer();
     if (Init && (!ForDup || (isa<CXXConstructExpr>(Init) &&
                              !CGF.isTrivialInitializer(Init)))) {
       LValue PrivateLValue = CGF.EmitLValueForField(PrivatesBase, *FI);
       if (const VarDecl *Elem = Pair.second.PrivateElemInit) {
         const VarDecl *OriginalVD = Pair.second.Original;
         // Check if the variable is the target-based BasePointersArray,
-        // PointersArray or SizesArray.
+        // PointersArray, SizesArray, or MappersArray.
         LValue SharedRefLValue;
         QualType Type = PrivateLValue.getType();
         const FieldDecl *SharedField = CapturesInfo.lookup(OriginalVD);
         if (IsTargetTask && !SharedField) {
           assert(isa<ImplicitParamDecl>(OriginalVD) &&
                  isa<CapturedDecl>(OriginalVD->getDeclContext()) &&
                  cast<CapturedDecl>(OriginalVD->getDeclContext())
                          ->getNumParams() == 0 &&

 }
 
 /// Emit the arguments to be passed to the runtime library based on the
 /// arrays of base pointers, pointers, sizes, map types, and mappers.
 static void emitOffloadingArraysArgument(
     CodeGenFunction &CGF, llvm::Value *&BasePointersArrayArg,
     llvm::Value *&PointersArrayArg, llvm::Value *&SizesArrayArg,
     llvm::Value *&MapTypesArrayArg, llvm::Value *&MappersArrayArg,
-    CGOpenMPRuntime::TargetDataInfo &Info) {
+    CGOpenMPRuntime::TargetDataInfo &Info, bool IsTask = false) {
   CodeGenModule &CGM = CGF.CGM;
   if (Info.NumberOfPtrs) {
     BasePointersArrayArg = CGF.Builder.CreateConstInBoundsGEP2_32(
         llvm::ArrayType::get(CGM.VoidPtrTy, Info.NumberOfPtrs),
         Info.BasePointersArray,
         /*Idx0=*/0, /*Idx1=*/0);
     PointersArrayArg = CGF.Builder.CreateConstInBoundsGEP2_32(
         llvm::ArrayType::get(CGM.VoidPtrTy, Info.NumberOfPtrs),
         Info.PointersArray,
         /*Idx0=*/0,
         /*Idx1=*/0);
     SizesArrayArg = CGF.Builder.CreateConstInBoundsGEP2_32(
         llvm::ArrayType::get(CGM.Int64Ty, Info.NumberOfPtrs), Info.SizesArray,
         /*Idx0=*/0, /*Idx1=*/0);
     MapTypesArrayArg = CGF.Builder.CreateConstInBoundsGEP2_32(
         llvm::ArrayType::get(CGM.Int64Ty, Info.NumberOfPtrs),
         Info.MapTypesArray,
         /*Idx0=*/0,
         /*Idx1=*/0);
+    // Always emit the mapper array address in case of a target task for
+    // privatization.
+    if (!IsTask && !Info.HasMapper)
+      MappersArrayArg = llvm::ConstantPointerNull::get(CGM.VoidPtrPtrTy);
+    else
       MappersArrayArg =
-        Info.HasMapper
-            ? CGF.Builder.CreatePointerCast(Info.MappersArray, CGM.VoidPtrPtrTy)
-            : llvm::ConstantPointerNull::get(CGM.VoidPtrPtrTy);
+          CGF.Builder.CreatePointerCast(Info.MappersArray, CGM.VoidPtrPtrTy);
   } else {
     BasePointersArrayArg = llvm::ConstantPointerNull::get(CGM.VoidPtrPtrTy);
     PointersArrayArg = llvm::ConstantPointerNull::get(CGM.VoidPtrPtrTy);
     SizesArrayArg = llvm::ConstantPointerNull::get(CGM.Int64Ty->getPointerTo());
     MapTypesArrayArg =
         llvm::ConstantPointerNull::get(CGM.Int64Ty->getPointerTo());
     MappersArrayArg = llvm::ConstantPointerNull::get(CGM.VoidPtrPtrTy);
   }
MEHandler.adjustMemberOfForLambdaCaptures(
         CombinedInfo.Types);
     // Map any list items in a map clause that were not captures because they
     // weren't referenced within the construct.
     MEHandler.generateAllInfo(CombinedInfo, MappedVarSet);
 
     TargetDataInfo Info;
     // Fill up the arrays and create the arguments.
     emitOffloadingArrays(CGF, CombinedInfo, Info);
-    emitOffloadingArraysArgument(CGF, Info.BasePointersArray,
-                                 Info.PointersArray, Info.SizesArray,
-                                 Info.MapTypesArray, Info.MappersArray, Info);
+    emitOffloadingArraysArgument(
+        CGF, Info.BasePointersArray, Info.PointersArray, Info.SizesArray,
+        Info.MapTypesArray, Info.MappersArray, Info, RequiresOuterTask);
     InputInfo.NumberOfTargetItems = Info.NumberOfPtrs;
     InputInfo.BasePointersArray =
         Address(Info.BasePointersArray, CGM.getPointerAlign());
     InputInfo.PointersArray =
         Address(Info.PointersArray, CGM.getPointerAlign());
     InputInfo.SizesArray = Address(Info.SizesArray, CGM.getPointerAlign());
     InputInfo.MappersArray = Address(Info.MappersArray, CGM.getPointerAlign());
     MapTypesArray = Info.MapTypesArray;
auto &&TargetThenGen = [this, &ThenGen, &D, &InputInfo, &MapTypesArray](
     MappableExprsHandler MEHandler(D, CGF);
     MEHandler.generateAllInfo(CombinedInfo);
 
     TargetDataInfo Info;
     // Fill up the arrays and create the arguments.
     emitOffloadingArrays(CGF, CombinedInfo, Info);
     emitOffloadingArraysArgument(CGF, Info.BasePointersArray,
                                  Info.PointersArray, Info.SizesArray,
-                                 Info.MapTypesArray, Info.MappersArray, Info);
+                                 Info.MapTypesArray, Info.MappersArray, Info,
+                                 D.hasClausesOfKind<OMPDependClause>());
     InputInfo.NumberOfTargetItems = Info.NumberOfPtrs;
     InputInfo.BasePointersArray =
         Address(Info.BasePointersArray, CGM.getPointerAlign());
     InputInfo.PointersArray =
         Address(Info.PointersArray, CGM.getPointerAlign());
     InputInfo.SizesArray =
         Address(Info.SizesArray, CGM.getPointerAlign());
     InputInfo.MappersArray = Address(Info.MappersArray, CGM.getPointerAlign());