[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

gregmiller added a comment. In D101030#2746957 , @jdoerfert wrote: > In D101030#2746770 , @gregmiller > wrote: > >> In D101030#2745513 , @jdoerfert >> wrote: >> >>> In D

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added a comment. In D101030#2746770 , @gregmiller wrote: > In D101030#2745513 , @jdoerfert > wrote: > >> In D101030#2745429 , @gregmiller >> wrote: >> >>> Hel

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

gregmiller added a comment. In D101030#2745513 , @jdoerfert wrote: > In D101030#2745429 , @gregmiller > wrote: > >> Hello, We are maintaining a downstream version of the monorepo based on the >> LLVM main branch

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added a comment. In D101030#2745429 , @gregmiller wrote: > Hello, We are maintaining a downstream version of the monorepo based on the > LLVM main branch. We have not transitioned to the new PM yet. In a recent > attempt to merge the latest u

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

gregmiller added a comment. Hello, We are maintaining a downstream version of the monorepo based on the LLVM main branch. We have not transitioned to the new PM yet. In a recent attempt to merge the latest upstream commits into our monorepo we came across the following test failures after your

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

This revision was automatically updated to reflect the committed changes. Closed by commit rGdf729e2b82b3: [OpenMP] Overhaul `declare target` handling (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert updated this revision to Diff 343290. jdoerfert added a comment. Address the last test failures (all ordering) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030 Files: clang/include/clang/Basic/

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert updated this revision to Diff 343285. jdoerfert added a comment. Reapply test changes accidentally dropped Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030 Files: clang/include/clang/Basic/Attr

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert updated this revision to Diff 343282. jdoerfert added a comment. Herald added a subscriber: jfb. Update tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030 Files: clang/include/clang/Basic/A

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added a comment. In D101030#2736508 , @jdoerfert wrote: > I'll wait for @ggeorgakoudis to update the tests with the script, then I'll > adjust all clang tests again. FWIW, this also fixes an issue in OpenMC where > declare target triggered this:

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added a subscriber: ggeorgakoudis. jdoerfert added a comment. I'll wait for @ggeorgakoudis to update the tests with the script, then I'll adjust all clang tests again. FWIW, this also fixes an issue in OpenMC where declare target triggered this: llvm-project/clang/lib/CodeGen/CodeGe

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev accepted this revision. ABataev added a comment. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert marked 7 inline comments as done. jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E);

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert updated this revision to Diff 342765. jdoerfert marked 4 inline comments as done. jdoerfert added a comment. Remove const cast, clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030 Files

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert updated this revision to Diff 342764. jdoerfert added a comment. Use mutateType, add test for static variable in function with allocate directive Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); ABataev wrote: > jdoerfert wrote: > > ABata

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); jdoerfert wrote: > ABataev wrote: > > jdoerfe

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); ABataev wrote: > jdoerfert wrote: > > ABata

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); jdoerfert wrote: > ABataev wrote: > > jdoerfe

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); ABataev wrote: > jdoerfert wrote: > > ABata

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); jdoerfert wrote: > ABataev wrote: > > Why nee

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); ABataev wrote: > Why need to remove constan

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618 +void CodeGenModule::EmitOMPAllocateDecl(const OMPAllocateDecl *D) { + for (Expr *E : const_cast(D)->varlists()) { +auto *DE = cast(E); Why need to remove constantness here? =

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert updated this revision to Diff 342419. jdoerfert added a comment. Fix tests by not always emitting a definition for omp allocate Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030 Files: clang/inc

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added a comment. I'm fixing the tests now, also realized that the `omp allocate` handling now always creates definitions, which is bad. I'll upload a new version soon, test case for this, which shows we also don't do a good job now, is here: https://godbolt.org/z/T9od5Mq7h Repository

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

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/D101030/new/ https://reviews.llvm.org/D101030 ___

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added a comment. In D101030#2713117 , @ABataev wrote: > Looks good, but would be nice to check with sanitizers that there are no > uses-after-delete. There is no heap allocation anymore but I guess I could have messed up the stack stuff. I do

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added a comment. Looks good, but would be nice to check with sanitizers that there are no uses-after-delete. Comment at: clang/lib/CodeGen/CGDecl.cpp:2618-2619 + for (const Expr *E : D->varlists()) { +auto *DE = cast(E); +auto *VD = cast(DE->getDecl()); +i

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert updated this revision to Diff 340126. jdoerfert added a comment. Avoid heap memory, copy when necessary, add documentation, address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101030/new/ https://reviews.llvm.org/D101030 Files

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added a comment. In D101030#2712738 , @jdoerfert wrote: > I'm testing a new version that does not allocate the structure on the heap. > Instead we copy it. It is really infrequent and it should be <40 bytes when > that happens, let's not over-op

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. I'm testing a new version that does not allocate the structure on the heap. Instead we copy it. It is really infrequent and it should be <40 bytes when that happens, let's not over-optimize ;) Repository: rG LLVM Github M

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added inline comments. Comment at: clang/lib/Parse/ParseOpenMP.cpp:2134-2151 +Sema::DeclareTargetContextInfo *DTCI = +new Sema::DeclareTargetContextInfo(DKind, DTLoc); +if (HasClauses) + ParseOMPDeclareTargetClauses(*DTCI); // Skip the last ann

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert added inline comments. Comment at: clang/lib/Parse/ParseOpenMP.cpp:2134-2151 +Sema::DeclareTargetContextInfo *DTCI = +new Sema::DeclareTargetContextInfo(DKind, DTLoc); +if (HasClauses) + ParseOMPDeclareTargetClauses(*DTCI); // Skip the last a

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added inline comments. Comment at: clang/lib/Parse/ParseOpenMP.cpp:2134-2151 +Sema::DeclareTargetContextInfo *DTCI = +new Sema::DeclareTargetContextInfo(DKind, DTLoc); +if (HasClauses) + ParseOMPDeclareTargetClauses(*DTCI); // Skip the last ann

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/lib/Parse/ParseOpenMP.cpp:2134-2151 +Sema::DeclareTargetContextInfo *DTCI = +new Sema::DeclareTargetContextInfo(DKind, DTLoc); +if (HasClauses) + ParseOMPDeclareTar

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added inline comments. Comment at: clang/lib/Parse/ParseOpenMP.cpp:2134-2151 +Sema::DeclareTargetContextInfo *DTCI = +new Sema::DeclareTargetContextInfo(DKind, DTLoc); +if (HasClauses) + ParseOMPDeclareTargetClauses(*DTCI); // Skip the last ann

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert marked an inline comment as done. jdoerfert added a comment. In D101030#2708277 , @ABataev wrote: > Could you check that it does not break the tests from > https://github.com/clang-ykt/omptests? IIRC, "ref" was introduced to fix a > bug with t

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

ABataev added a comment. Could you check that it does not break the tests from https://github.com/clang-ykt/omptests? IIRC, "ref" was introduced to fix a bug with too early optimizations of the declare target variables defined in other modules. Check `t-same-name-definitions` especially, though

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert created this revision. jdoerfert added reviewers: ABataev, grokos, dreachem, cchen, JonChesterfield. Herald added subscribers: guansong, yaxunl. Herald added a reviewer: bollu. Herald added a reviewer: aaron.ballman. jdoerfert requested review of this revision. Herald added subscribers: l