[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

This revision was automatically updated to reflect the committed changes. Closed by commit rG002d61db2b77: [OpenMP] Fix `present` for exit from `omp target data` (authored by jdenny). Changed prior to commit: https://reviews.llvm.org/D84422?vs=281330&id=283224#toc Repository: rG LLVM Github

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny marked an inline comment as done. jdenny added a comment. Thanks for the review. As discussed during the 7/29 call, I'll wait to push until we're sure about what the OpenMP committee intended here. I'm pursuing this and will report back when I have more information. CHANGES SINCE LAST

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84422/new/ https://reviews.llvm.org/D84422 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8843 llvm::Value *&MapTypesArrayArg, llvm::Value *&MappersArrayArg, -CGOpenMPRuntime::TargetDataInfo &Info) { +CGOpenMPRuntime::TargetDataInfo &In

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8843 llvm::Value *&MapTypesArrayArg, llvm::Value *&MappersArrayArg, -CGOpenMPRuntime::TargetDataInfo &Info) { +CGOpenMPRuntime::TargetDataInfo &Info, bool ForEndCall = false) { + assert

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny updated this revision to Diff 281330. jdenny added a comment. Replaced `SeparateBeginEnd` parameter with new `TargetDataInfo` field as requested. Rebased. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84422/new/ https://reviews.llvm.org/D84422 Files: clang/lib/CodeGen/CGOpen

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8686 +CodeGenFunction &CGF, MappableExprsHandler::MapCombinedInfoTy &CombinedInfo, +CGOpenMPRuntime::TargetDataInfo &Info, bool SeparateBeginEnd) { CodeGenModule &CGM = CGF.CGM; -

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. This looks much better now. I don't have any other comments. Since this patch is now essentially a clang-only patch, I'll let @ABataev accept it or post comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84422/new/ https://reviews.llvm.org/D84422 __

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny updated this revision to Diff 281067. jdenny edited the summary of this revision. jdenny added a comment. Rewrite patch as discussed: instead of generating different runtime calls for the end of an `omp target data` vs. the beginning of an `omp target exit data` so that the runtime can de

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny added a comment. In D84422#2176802 , @grokos wrote: > In D84422#2173500 , @jdenny wrote: > > > I've added a comment to the runtime code that performs the check. As you > > can see, the check is performed re

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. In D84422#2173500 , @jdenny wrote: > I've added a comment to the runtime code that performs the check. As you can > see, the check is performed regardless. It's just a question of whether the > runtime treats it as an error. I

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny marked an inline comment as done. jdenny added a comment. In D84422#2173449 , @RaviNarayanaswamy wrote: > In D84422#2173372 , @jdenny wrote: > > > In D84422#2172898 ,

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

RaviNarayanaswamy added a comment. In D84422#2173372 , @jdenny wrote: > In D84422#2172898 , @jdenny wrote: > > > Has anyone clarified the motivation for this behavior? > > > I meant, is there any insight into why th

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny added a comment. In D84422#2172898 , @jdenny wrote: > Has anyone clarified the motivation for this behavior? I meant, is there any insight into why the spec specifies this behavior? In D84422#2172926 , @gr

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. So let's proceed with the patch. Instead of introducing new API functions and making all these changes in all these files, wouldn't it be easier if we just unset the `PRESENT` flag from arg_types in clang when we generate the call to `__tgt_target_data_end_*` if we are

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny added a comment. In D84422#2170702 , @RaviNarayanaswamy wrote: > In D84422#2170667 , @grokos wrote: > > > So is the test case that motivated this patch illegal OpenMP code? > > > > #pragma omp target enter

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

RaviNarayanaswamy added a comment. In D84422#2170667 , @grokos wrote: > So is the test case that motivated this patch illegal OpenMP code? > > #pragma omp target enter data map(alloc:i) > #pragma omp target data map(present, alloc: i) > { > #prag

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. So is the test case that motivated this patch illegal OpenMP code? #pragma omp target enter data map(alloc:i) #pragma omp target data map(present, alloc: i) { #pragma omp target exit data map(delete:i) } // fails presence check here Repository: rG LLVM Gith

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

RaviNarayanaswamy added a comment. In D84422#2170285 , @grokos wrote: > What confuses me about this interpretation of the standard is the > inconsistency at `data exit`. So if we have an explicit `omp target exit data > map(present...)` then we should re

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny added a comment. I don't know if the OpenMP committee has any documented rationale for this behavior. I can say that the OpenACC committee is considering the same semantics. However, the issues to consider are not identical. For example, OpenACC has a separate structured reference cou

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

grokos added a comment. What confuses me about this interpretation of the standard is the inconsistency at `data exit`. So if we have an explicit `omp target exit data map(present...)` then we should respect the "present" semantics, whereas when we have a scoped data exit: #pragma omp target

[PATCH] D84422: [OpenMP] Fix `present` for exit from `omp target data`

jdenny created this revision. jdenny added reviewers: grokos, ABataev, jdoerfert. Herald added subscribers: llvm-commits, openmp-commits, cfe-commits, sstefan1, guansong, yaxunl. Herald added projects: clang, OpenMP, LLVM. Without this patch, the following example fails but shouldn't according to