[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert updated this revision to Diff 229448. jdoerfert marked 5 inline comments as done. jdoerfert added a comment. Addressed comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib/C

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1499-1509 +OMPBuilder->pushFinalizationCB(std::move(FI)); + } + CGOpenMPOutlinedRegionInfo CGInfo(*CS, ThreadIDVar, CodeGen, InnermostKind, HasCancel, O

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71 + void pushFinalizationCB(FinalizationInfo &&FI) { +FinalizationStack.emplace_back(std::move(FI)); + } ABataev wrote:

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert updated this revision to Diff 229573. jdoerfert added a comment. Replace conditional by RAII object Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert updated this revision to Diff 229582. jdoerfert marked an inline comment as done. jdoerfert added a comment. Make it a specialized push-pop RAII object (just moved code) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://rev

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71 + void pushFinalizationCB(FinalizationInfo &&FI) { +FinalizationStack.emplace_back(std::move(FI)); + } ABataev wrote:

[PATCH] D70245: [OPENMP50]Add device/kind context selector support.

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:11247 +RHSIsSubsetOfLHS = true; + } else { +for (const OMPContextSelectorData &Data : LHS) { Nit: `bool RHSIsSubsetOfLHS = isStrictSubset(RHS, LHS)` saves you a line later

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked 4 inline comments as done. jdoerfert added a comment. ping Comment at: llvm/include/llvm/Frontend/OpenMPIRBuilder.h:69-71 + void pushFinalizationCB(FinalizationInfo &&FI) { +FinalizationStack.emplace_back(std::move(FI)); + } jdoerfert wro

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D70245: [OPENMP50]Add device/kind context selector support.

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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert marked 4 inline comments as done. jdoerfert added a comment. Anything else? Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:152-155 + std::string LineStr = std::to_string(DIL->getLine()); + std::string ColumnStr = std::to_string(DIL->getColumn()); + std::string S

[PATCH] D70549: [OPENMP]Fix PR41826: symbols visibility in device code.

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

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert updated this revision to Diff 230928. jdoerfert marked 4 inline comments as done. jdoerfert added a comment. use streams, remove caching Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 Files: ll

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert added a comment. @ABataev @JonChesterfield anything else blocking this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 ___ cfe-commits mailin

[PATCH] D70739: [OPENMP50]Add device/isa context selector support.

jdoerfert added a comment. Thanks for working on this! One nit and one concern below. Comment at: clang/include/clang/Sema/Sema.h:9320 using OMPCtxSelectorData = - OpenMPCtxSelectorData, ExprResult>; + OpenMPCtxSelectorData, ExprResult>; I would

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

jdoerfert marked 4 inline comments as done. jdoerfert added inline comments. Comment at: clang/include/clang/Basic/OpenMPKinds.h:23 /// OpenMP directives. -enum OpenMPDirectiveKind { -#define OPENMP_DIRECTIVE(Name) \ - OMPD_##Name, -#define OPENMP_DIRECTIVE_EXT(Name, Str) \ -

[PATCH] D70739: [OPENMP50]Add device/isa context selector support.

jdoerfert added inline comments. Comment at: clang/include/clang/Sema/Sema.h:9320 using OMPCtxSelectorData = - OpenMPCtxSelectorData, ExprResult>; + OpenMPCtxSelectorData, ExprResult>; ABataev wrote: > jdoerfert wrote: > > I would like to avoid the

[PATCH] D70739: [OPENMP50]Add device/isa context selector support.

jdoerfert added inline comments. Comment at: clang/include/clang/Sema/Sema.h:9320 using OMPCtxSelectorData = - OpenMPCtxSelectorData, ExprResult>; + OpenMPCtxSelectorData, ExprResult>; ABataev wrote: > jdoerfert wrote: > > ABataev wrote: > > > jdoe

[PATCH] D70804: [Frontend] Allow OpenMP offloading to aarch64

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

[PATCH] D70799: [OpenMP] Lower taskyield using OpenMP IR Builder

jdoerfert accepted this revision. jdoerfert added a subscriber: fghanim. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM. @fghanim started a spreadsheet to coordinate our work: https://docs.google.com/spreadsheets/d/1FvHPuSkGbl4mQZRAwCIndvQx9dQboffiD-xD0oqxgU0/ed

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

jdoerfert marked 3 inline comments as done. jdoerfert added a comment. In D69922#1742392 , @ABataev wrote: > In D69922#1741659 , @jdoerfert wrote: > > > Use IRBuilder for cancel barriers > > > In general, it would b

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert marked an inline comment as done. jdoerfert added a comment. In D69785#1762438 , @JonChesterfield wrote: > I'd very much like this to land soon. It's the prereq for a lot of other > patches and the code looks good. > > It's tricky to test the i

[PATCH] D70366: Add new 'flatten' LLVM attribute to fix clang's 'flatten' function attribute

jdoerfert added a comment. We need more tests here. For one, the flatten attribute is not necessary to pass the test. Second, we need to check the corner cases, e.g. reduction with different cycle lengths. Comment at: llvm/docs/LangRef.rst:1428 can prove that the call/in

[PATCH] D70973: [OPENMP50]Treat context selectors as expressions, not just strings.

jdoerfert added a comment. Thx for this patch. Only two minor comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1230 - "unknown '%0' device kind trait in the 'device' context selector set, expected" - " one of 'host', 'nohost', 'cpu', 'gpu' or 'fpga'">;

[PATCH] D70973: [OPENMP50]Treat context selectors as expressions, not just strings.

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1230 - "unknown '%0' device kind trait in the 'device' context selector set, expected" - " one of 'host', 'nohost', 'cpu', 'gpu' or 'fpga'">; ABataev wrote: > jdoe

[PATCH] D70973: [OPENMP50]Treat context selectors as expressions, not just strings.

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1230 - "unknown '%0' device kind trait in the 'device' context selector set, expected" - " one of 'host', 'nohost', 'cpu', 'gpu' or 'fpga'">; ABataev wrote: > jdoe

[PATCH] D70973: [OPENMP50]Treat context selectors as expressions, not just strings.

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1230 - "unknown '%0' device kind trait in the 'device' context selector set, expected" - " one of 'host', 'nohost', 'cpu', 'gpu' or 'fpga'">; ABataev wrote: > jdoe

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: s.egerton, guansong, bollu, simoncook, fedor.sergeev, aheejin, rampitec. Herald added a project: cla

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked 4 inline comments as done. jdoerfert added inline comments. Comment at: clang/lib/AST/StmtOpenMP.cpp:2243 } + +// TODO: We have various representations for the same data, it might help to This code was basically only moved, not written for this

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232750. jdoerfert added a comment. Add (missing) include. (Worked locally just fine). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#177 , @ABataev wrote: > I read the spec and don't think that we need all this complex stuff for the > implementation. Yiu need judt to check at the codegen phase if the function > must be emitted or not. We don't

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#1774469 , @ABataev wrote: > In D71179#1774448 , @jdoerfert wrote: > > > In D71179#177 , @ABataev wrote: > > > > > I read the spec and

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#1774471 , @ABataev wrote: > They do this because they have several function definitions with the same > name. In our case, we have several different functions with different names > and for us no need to worry about o

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked 11 inline comments as done. jdoerfert added inline comments. Comment at: clang/lib/AST/StmtOpenMP.cpp:2243 } + +// TODO: We have various representations for the same data, it might help to ABataev wrote: > jdoerfert wrote: > > This code was basi

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked 5 inline comments as done. jdoerfert added a comment. > @jdoerfert , how does the ".ompvariant" work with external functions? I see > the part of the spec which says, "The symbol name of a function definition > that appears between a begin declare variant...", but, if we append

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. > @jdoerfert , also, do we have tests that can go into the test suite / > libomptarget regression tests demonstrating the collection of problems people > have currently opened bugs on regarding math.h? I recall we still had > problems with host code needing the long-d

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232902. jdoerfert added a comment. Add one more test sin(long double), and fix some rebase issues Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/inclu

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232906. jdoerfert added a comment. minor fix Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST/Decl.h clang/include/clang/AST/StmtOpe

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added inline comments. Comment at: clang/lib/Headers/__clang_cuda_cmath.h:72 -#ifndef _OPENMP -__DEVICE__ int fpclassify(float __x) { - return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, tra wrote: > Please keep fpclassify in pla

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 232909. jdoerfert marked 6 inline comments as done. jdoerfert added a comment. Undo math function removal (fpclassify & scalblnf), reorder includes (host first) The latter is the "natural way" but also necessary because fpclassify uses macros and we did not

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. >> This is neither true, nor relevant. It is not true because OpenMP 5.0 >> declare variant is so broken it cannot be used for what it was intended for. >> That means people (as for example we for math) will inevitably use begin/end >> declare variant. > > I rather d

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. > Not always. If we see that the context selector does not match, we can skip > everything between begin/end. It means exactly what I said - multiversioning > is needed only for `construct` because all other traits can be easily > resolved at the compile time. General

[PATCH] D71225: [OpenMP][WIP] atomic update only evaluate once for expression having side effect

jdoerfert added a comment. > • For forms that allow multiple occurrences of x, the number of times that x > is evaluated is unspecified. x here is `iarr[foo(), foo(), 0]`, if I'm not mistaken. Assuming not, any multiple of 2 is a fine amount of evaluations for `foo`. I'd vote for a warning her

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. In D71179#1776046 , @ABataev wrote: > In D71179#1776034 , @jdoerfert wrote: > > > > Not always. If we see that the context selector does not match, we can > > > skip everything between be

[PATCH] D71225: [OpenMP][WIP] atomic update only evaluate once for expression having side effect

jdoerfert added a comment. In D71225#1776105 , @cchen wrote: > Oops, accidentally remove my own comment. I'm not sure why `iarr[foo(), > foo(), 0]` violate the rule since it will be evaluated as `iarr[0]`, and the > counter `foo()` modified is also in th

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/lib/Headers/__clang_cuda_math_forward_declares.h:41 __DEVICE__ long abs(long); __DEVICE__ long long abs(long long); -__DEVICE__ double abs(double); I have to double c

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. > You're doing absolutely the same thing as the original declare variant > implementation. I don't think so but if you do why do you oppose this approach? > And I don't think it would be correct to add them as multiversiin variants to > the original function. Why wo

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

jdoerfert updated this revision to Diff 232984. jdoerfert added a comment. Rebase and creation of libFrontendOpenMP Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69853/new/ https://reviews.llvm.org/D69853 Files: clang/include/clang/AST/OpenMPCla

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

jdoerfert updated this revision to Diff 232987. jdoerfert added a comment. Add missing files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69853/new/ https://reviews.llvm.org/D69853 Files: clang/include/clang/AST/OpenMPClause.h clang/include/c

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

This revision was automatically updated to reflect the committed changes. Closed by commit rGeb3e81f43f01: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h (authored by jdoerfert). Changed prior to commit: https://reviews.llvm.org/D69853?vs=232987&id=233003#toc Repository: rG LLVM Github M

[PATCH] D70245: [OPENMP50]Add device/kind context selector support.

jdoerfert added a comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70245/new/ https://reviews.llvm.org/D70245 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: s.egerton, guansong, bollu, simoncook, fedor.sergeev, aheejin, rampitec, jholewinski. Herald added a

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert added a comment. So, D71241 shows how declare variant (5.0) would look like if we implement it through SemaLookup. I will actually revisit this patch tomorrow as I might be able to make it even simpler. (D71241 is sav

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. In D71241#1776798 , @ABataev wrote: > You're merging different functions as multiversiin variants. I don't think > this right to overcomplicate the semantics of multiversion functions

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert marked an inline comment as done. jdoerfert added a comment. In D71241#109 , @ABataev wrote: > In D71241#1777661 , @jdoerfert wrote: > > > In D71241#1776798 , @

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 233234. jdoerfert added a comment. Diff against TOT Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST/Decl.h clang/include/clang/AST/

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 233232. jdoerfert added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Consistent overload based solution. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://r

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. >> There is no evidence that this is more complicated. By all measures, this is >> less complicated (see also below). It is also actually doing the right thing >> when it comes to code emission. Take https://godbolt.org/z/sJiP3B for >> example. The calls are wrong and

[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

jdoerfert updated this revision to Diff 233238. jdoerfert added a comment. Fix math problem Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71179/new/ https://reviews.llvm.org/D71179 Files: clang/include/clang/AST/Decl.h clang/include/clang/AST/

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1778736 , @ABataev wrote: > In D71241#1778717 , @jdoerfert wrote: > > > >> There is no evidence that this is more complicated. By all measures, > > >> this is less complicated (

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert updated this revision to Diff 233409. jdoerfert added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 Files: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h llvm/include/llv

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert updated this revision to Diff 233418. jdoerfert added a comment. Adjust path Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 Files: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h llvm/includ

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert marked 6 inline comments as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:74 +/// +///{ +namespace types { ABataev wrote: > Extra comments? I don't know what you want to tell me. C

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert updated this revision to Diff 233429. jdoerfert marked 2 inline comments as done. jdoerfert added a comment. Remove call caching in anticipation of D69930 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D6978

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

jdoerfert marked 2 inline comments as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:74 +/// +///{ +namespace types { ABataev wrote: > jdoerfert wrote: > > ABataev wrote: > > > Extra comments? > > I don't know

[PATCH] D69785: [OpenMP] Introduce the OpenMP-IR-Builder

This revision was automatically updated to reflect the committed changes. Closed by commit rGd23c61490c28: [OpenMP] Introduce the OpenMP-IR-Builder (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llv

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

jdoerfert updated this revision to Diff 233448. jdoerfert marked 3 inline comments as done. jdoerfert added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69922/new/ https://reviews.llvm.org/D69922 Files: clang/include/clang/Bas

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

This revision was automatically updated to reflect the committed changes. Closed by commit rGb3c06db45611: [OpenMP] Use the OpenMP-IR-Builder (authored by jdoerfert). Changed prior to commit: https://reviews.llvm.org/D69922?vs=233448&id=233458#toc Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D69922: [OpenMP] Use the OpenMP-IR-Builder

jdoerfert added a comment. > I applied D69853 , D69785 > , D69922 > to my local build and found that D69922 is > referring to OpenMPIRBuilder.h in llvm/Frontend

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert updated this revision to Diff 233462. jdoerfert added a comment. rebase + ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp llvm/include/llvm/Fr

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. In D70258#1780611 , @ABataev wrote: > Tests? This changes the implementation and does not add features. Thus, this is covered by the existing tests for the functionality, e.g. the un

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert updated this revision to Diff 233472. jdoerfert marked an inline comment as done. jdoerfert added a comment. Update the unit test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 Files: clang/lib

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51 + /// at the time, and location, the callback is invoked. + using FinalizeCallbackTy = std::function; + ABataev wrote: >

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1779097 , @ABataev wrote: > In D71241#1778963 , @jdoerfert wrote: > > > In D71241#1778736 , @ABataev wrote: > > > > > In D71241#1778717 <

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51 + /// at the time, and location, the callback is invoked. + using FinalizeCallbackTy = std::function; + ABataev wrote: >

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1781955 , @ABataev wrote: > In D71241#1780715 , @jdoerfert wrote: > > > In D71241#1779097 , @ABataev wrote: > > > > > In D71241#1778963 <

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51 + /// at the time, and location, the callback is invoked. + using FinalizeCallbackTy = std::function; + ABataev wrote: >

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1782173 , @ABataev wrote: > In D71241#1782157 , @jdoerfert wrote: > > > In D71241#1781955 , @ABataev wrote: > > > > > In D71241#1780715 <

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:51 + /// at the time, and location, the callback is invoked. + using FinalizeCallbackTy = std::function; + hfinkel wrote: >

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. >> In fact, the current solution will disregard the `used` attribute here and >> not emit the function, which is bad. The Sema solution will dispatch the >> right calls and honor the `used` attribute properly. > > Nope, the function is emitted but as an alias to the f

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1782460 , @JonChesterfield wrote: > > https://clang.llvm.org/docs/InternalsManual.html#the-ast-library > > > > Faithfulness¶ > > The AST intends to provide a representation of the program that is > > faithful to

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1782614 , @ABataev wrote: > Actually, early resolution will break tbe tools, not help them. It will > definitely break clangd, for example. The user will try to navigate to > originally called function `base` and inst

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1782668 , @ABataev wrote: > In D71241#1782650 , @jdoerfert wrote: > > > While we talk a lot about what you think is bad about this solution it > > seems we ignore the problems i

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1782917 , @ABataev wrote: > In D71241#1782898 , @JonChesterfield > wrote: > > > In D71241#1782846 , @ABataev wrote: > > > > > But I sugg

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1783362 , @ABataev wrote: > >>> - Take https://godbolt.org/z/2evvtN which shows that the alias solution > >>> is incompatible with linking. > >> > >> Undefined behavior according to the standard. > > > > I don't thin

[PATCH] D71475: [WIP][OPENMP] Try to fix linear clause crash by emitting alloca for step

jdoerfert added a comment. Your commit message example lacks the #pragma. What if you add k to the list of explicit firstprivate? (I mean, you can try it in C first). And how do I reproduce the crash? I tried: https://godbolt.org/z/FDPSnA Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D71475: [WIP][OPENMP] Try to fix linear clause crash by emitting alloca for step

jdoerfert added a comment. In D71475#1783928 , @cchen wrote: > In D71475#1783834 , @jdoerfert wrote: > > > Your commit message example lacks the #pragma. > > > > What if you add k to the list of explicit firstprivat

[PATCH] D71475: [WIP][OPENMP] Try to fix linear clause crash by emitting alloca for step

jdoerfert added a comment. What is the output when you run the example with `k` in `lastprivate` or `reduction`? In D71475#1784173 , @cchen wrote: > Doing this still fail the assertion since we still don't have the variable > inside > CapturedStmt. S

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1786530 , @ABataev wrote: > Most probably, we can use this solution without adding a new expression. > `DeclRefExpr` class can contain 2 decls: FoundDecl and the Decl being used. > You can use FoundDecl to point to th

[PATCH] D71475: [WIP][OPENMP] Try to fix linear clause crash by emitting alloca for step

jdoerfert added a comment. In D71475#1784312 , @cchen wrote: > In D71475#1784284 , @jdoerfert wrote: > > > What is the output when you run the example with `k` in `lastprivate` or > > `reduction`? > > > I actually

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert added a comment. ping? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ https://reviews.llvm.org/D70258 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1787265 , @hfinkel wrote: > In D71241#1786959 , @jdoerfert wrote: > > > In D71241#1786530 , @ABataev wrote: > > > > > Most probably, we c

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1787888 , @ABataev wrote: > Hal, are we going to support something like this? I'm not Hal but I will answer anyway. > void cpu() { asm("nop"); } > > #pragma omp declare variant(cpu) match(device = {kind(cpu)})

[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

jdoerfert added a comment. In D71241#1787652 , @hfinkel wrote: > In D71241#1787571 , @ABataev wrote: > > > In D71241#1787265 , @hfinkel wrote: > > > > > In D71241#1786959

[PATCH] D71475: [WIP][OPENMP] Try to fix linear clause crash by emitting alloca for step

jdoerfert added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1128 + (A == OMPC_firstprivate && (Data.Attributes == OMPC_lastprivate || + Data.Attributes == OMPC_linear)) || (A == OMPC_lastprivate && Data.At

[PATCH] D71475: [WIP][OPENMP] Try to fix linear clause crash by emitting alloca for step

jdoerfert added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1128 + (A == OMPC_firstprivate && (Data.Attributes == OMPC_lastprivate || + Data.Attributes == OMPC_linear)) || (A == OMPC_lastprivate && Data.At

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert added a subscriber: rjmccall. jdoerfert added a comment. In D70258#1788305 , @rjmccall wrote: > I opposed the creation of this library on these terms in the first place, so > I'm pretty sure I'm not on the hook to review it. That's fine with m

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert added a comment. In D70258#1788427 , @rjmccall wrote: > In D70258#1788396 , @jdoerfert wrote: > > > In D70258#1788305 , @rjmccall > > wrote: > > > > > Introducing

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert added a comment. In D70258#1788825 , @rjmccall wrote: > So it's never that OpenMP has things that need to be finalized before exiting > (e.g. if something in frontend-emitted code throws an exception), it's just > that OpenMP might need to gene

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

jdoerfert added a comment. In D70258#1788861 , @rjmccall wrote: > That's what I figured. Just to say this again: Current OpenMP code generation keeps a second stack for exactly the same purpose as the one introduced here. > The thing is that that neces

<    9   10   11   12   13   14   15   16   17   18   >