[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

This revision was automatically updated to reflect the committed changes. jdenny marked 2 inline comments as done. Closed by commit rL369619: [OpenMP] Permit map with DSA on combined directive (authored by jdenny, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commit

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny marked 4 inline comments as done. jdenny added a comment. Thanks. Comment at: clang/lib/Sema/SemaExpr.cpp:17748 ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy); -} \ No newline at end of file +} ABataev wrote: > jdenny wrote: > >

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG Comment at: clang/lib/Sema/SemaExpr.cpp:17749 } \ No newline at end of file Still marked as changed code, better to restore it completely. CHANGES S

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:17748 ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy); -} \ No newline at end of file +} jdenny wrote: > ABataev wrote: > > Restore original code here > OK, I did. Wh

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:17748 ObjCAvailabilityCheckExpr(Version, AtLoc, RParen, Context.BoolTy); -} \ No newline at end of file +} ABataev wrote: > Restore original code here OK, I did. What's the reason for n

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 216502. jdenny marked 8 inline comments as done. jdenny added a comment. Make suggested changes to default arguments, comments on literals, and parameter names. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65835/new/ https://reviews.llvm.org/D65835

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1419 + RecordDecl *RD, CapturedRegionKind K, + unsigned CaptureLevel); Better to use `OpenMPCaptureLevel` since this param is

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 216469. jdenny marked 7 inline comments as done. jdenny set the repository for this revision to rG LLVM Github Monorepo. jdenny added a comment. Make suggested changes for passing around the capture level. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2108 void Sema::PushCapturedRegionScope(Scope *S, CapturedDecl *CD, RecordDecl *RD, CapturedRegionKind K) { + CapturedRegionScopeInfo *CSI = new CapturedRegionScopeInfo( -

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2108 void Sema::PushCapturedRegionScope(Scope *S, CapturedDecl *CD, RecordDecl *RD, CapturedRegionKind K) { + CapturedRegionScopeInfo *

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2108 void Sema::PushCapturedRegionScope(Scope *S, CapturedDecl *CD, RecordDecl *RD, CapturedRegionKind K) { + CapturedRegionScopeInfo *CSI = new CapturedRegionScopeInfo( -

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2108 void Sema::PushCapturedRegionScope(Scope *S, CapturedDecl *CD, RecordDecl *RD, CapturedRegionKind K) { + CapturedRegionScopeInfo *CSI = new CapturedRegionScopeInfo( -

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: clang/lib/Sema/Sema.cpp:2108 void Sema::PushCapturedRegionScope(Scope *S, CapturedDecl *CD, RecordDecl *RD, CapturedRegionKind K) { + CapturedRegionScopeInfo *

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added inline comments. Comment at: clang/include/clang/Sema/ScopeInfo.h:763 RecordDecl *RD, ImplicitParamDecl *Context, CapturedRegionKind K, unsigned OpenMPLevel) : CapturingScopeInfo(Diag, ImpCap_CapturedRegio

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 216444. jdenny edited the summary of this revision. jdenny added a comment. Restore previous version of patch, and rebase. I tried, and this patch is not sufficient to fix PR40253. If there are indeed common changes, it seems it's just as well to put them he

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1639739 , @jdenny wrote: > In D65835#1639700 , @ABataev wrote: > > > Chwck 2 ifs: `if (IsVariableUsedInMapClause)` and the second `if (IsByRef > > && Ty.getNonReferenceType()->isS

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1639700 , @ABataev wrote: > Chwck 2 ifs: `if (IsVariableUsedInMapClause)` and the second `if (IsByRef && > Ty.getNonReferenceType()->isScalarType())`. If the variable is mapped, > `IsByRef` is set to `true`, but later it

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1639685 , @jdenny wrote: > In D65835#1639622 , @ABataev wrote: > > > We don't need this new level counter to correctly handle this situation. > > Just check for the combined targe

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1639622 , @ABataev wrote: > We don't need this new level counter to correctly handle this situation. Just > check for the combined target directive in `Sema::isOpenMPCapturedByRef` and > return true if it is mapped as to

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1639617 , @jdenny wrote: > In D65835#1639612 , @ABataev wrote: > > > In D65835#1639593 , @jdenny wrote: > > > > > In D65835#1639585

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1639612 , @ABataev wrote: > In D65835#1639593 , @jdenny wrote: > > > In D65835#1639585 , @ABataev wrote: > > > > > In D65835#1639584

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1639593 , @jdenny wrote: > In D65835#1639585 , @ABataev wrote: > > > In D65835#1639584 , @jdenny wrote: > > > > > I want to be sure we're o

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1639585 , @ABataev wrote: > In D65835#1639584 , @jdenny wrote: > > > I want to be sure we're on the same page. Due to the changes I just backed > > out, the following two examples

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev requested changes to this revision. ABataev added a comment. This revision now requires changes to proceed. Need to fix the mapping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65835/new/ https://reviews.llvm.org/D65835

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1639584 , @jdenny wrote: > I want to be sure we're on the same page. Due to the changes I just backed > out, the following two examples now generate different code: > > int a = 0; > #pragma omp target map(a) > #pr

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. I want to be sure we're on the same page. Due to the changes I just backed out, the following two examples now generate different code: int a = 0; #pragma omp target map(a) #pragma omp teams firstprivate(a) ; int a = 0; #pragma omp target teams firstpriv

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65835/new/ https://reviews.llvm.org/D65835 ___

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 216420. jdenny edited the summary of this revision. jdenny set the repository for this revision to rG LLVM Github Monorepo. jdenny added a comment. As requested, backed out the changes related to firstprivate scalars, and updated tests. Repository: rG LLVM

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1630732 , @jdenny wrote: > I think you're referring to stuff like `OpenMPCaptureLevel` in `ScopeInfo.h`. > I wrote those changes for this patch first, as can be seen in phabricator > history. I needed them for the oth

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65835/new/ https://reviews.llvm.org/D65835 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. I think you're referring to stuff like `OpenMPCaptureLevel` in `ScopeInfo.h`. I wrote those changes for this patch first, as can be seen in phabricator history. I needed them for the other patch too, and I thought we were going to end up applying that patch first, so I

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. Why there are the changes from the another patch? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65835/new/ https://reviews.llvm.org/D65835 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 215298. jdenny added a comment. Rebase. Add more tests, as requested at D66247#1630441 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65835/new/ https://reviews.llvm.org/D65835 Files: clang/include/clang

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624659 , @ABataev wrote: > In D65835#1624629 , @jdenny wrote: > > > In D65835#1624624 , @ABataev wrote: > > > > > In D65835#1624619

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1624629 , @jdenny wrote: > In D65835#1624624 , @ABataev wrote: > > > In D65835#1624619 , @jdenny wrote: > > > > > In D65835#1624617

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624624 , @ABataev wrote: > In D65835#1624619 , @jdenny wrote: > > > In D65835#1624617 , @ABataev wrote: > > > > > Target teams private map

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1624619 , @jdenny wrote: > In D65835#1624617 , @ABataev wrote: > > > Target teams private map will produce extra private in target context, > > other constructs either. > > > Here

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624617 , @ABataev wrote: > Target teams private map will produce extra private in target context, other > constructs either. Here's what I tried: int a; #pragma omp target teams private(a) map(a) ; The same c

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1624616 , @jdenny wrote: > In D65835#1624615 , @ABataev wrote: > > > This is wrong. It affects all possible combinations and not only fof scalar > > types, all of them are affecte

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624615 , @ABataev wrote: > This is wrong. It affects all possible combinations and not only fof scalar > types, all of them are affected. Are you saying the patch isn't sufficient because other types need to be fixed

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1624610 , @jdenny wrote: > In D65835#1624477 , @ABataev wrote: > > > Try something like `target parallel firstprivate (a) map(a)`. Currently it > > will create a firstprivate copy

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 214559. jdenny marked 3 inline comments as done. jdenny edited the summary of this revision. jdenny set the repository for this revision to rG LLVM Github Monorepo. jdenny added a comment. In D65835#1624477 , @ABataev wr

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

ABataev added a comment. In D65835#1624471 , @jdenny wrote: > I made the following changes, as suggested: > > - Add back restriction for OpenMP < 5.0. > - Remove `is_device_ptr` changes. > > Alexey, you said: > > > Plus, these changes are not enough to s

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 214542. jdenny retitled this revision from "[OpenMP] Fix map/is_device_ptr with DSA on combined directive" to "[OpenMP] Permit map with DSA on combined directive". jdenny edited the summary of this revision. jdenny added a comment. I made the following changes