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

jdoerfert marked an inline comment as done. jdoerfert added a comment. In D69785#1734205 , @ABataev wrote: > Also, I think it would better to split LLVM part and clang part into separate > patches. What do you mean exactly and why?

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

jdoerfert marked 4 inline comments as done. jdoerfert added inline comments. Comment at: clang/test/Driver/fopenmp.c:130-131 +// CHECK-CC1-OPENMPIRBUILDER: "-cc1" +// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp" +// CHECK-CC1-OPENMPIRBUILDER: "-fopenmp-enable-irbuilder" +//

[PATCH] D69952: [OPENMP50]Generalize handling of context matching/scoring.

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/OpenMPKinds.h:43 +/// Struct to store the context selectors info. +template > +struct OpenMPCtxSelectorData { ABataev wrote: > jdoerfert wrote: > > ABataev wrote: > > > jdoerfert wrote: > > >

[PATCH] D69984: [OpenMP][Opt] Annotate known runtime functions and deduplicate more

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added a project: LLVM. jdoerfert removed a subscriber: cfe-commits. jdoerfert added a parent revision: D69930: [OpenMP]

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

jdoerfert updated this revision to Diff 228331. jdoerfert marked 2 inline comments as done. jdoerfert added a comment. Herald added a project: LLVM. Adjust tests as requested Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69922/new/

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a comment. In D69498#1726610 , @mehdi_amini wrote: > > The short version is the fact that most developers aren't aware of and > > don't understand the subtleties of convergence, and making sure the > > front-end does something in all

[PATCH] D69666: clang: Fix assert on void pointer arithmetic with address_space

jdoerfert added a comment. Communication problem: > This attempted to always use the default address space void pointer type > instead of preserving the source address space. Where "This" is the old code. Replace "This" in the commit message with some more descriptive wording and the

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a subscriber: hfinkel. jdoerfert added a comment. In D69498#1727419 , @mehdi_amini wrote: > In D69498#1727080 , @jdoerfert wrote: > > > Let me quote @arsenm here because this is so important:

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a comment. In D69498#1727626 , @mehdi_amini wrote: > In D69498#1727546 , @jdoerfert wrote: > > > In D69498#1727419 , @mehdi_amini > > wrote: > > > > > In

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

jdoerfert added inline comments. Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:370 +: nullptr; +} return nullptr; dim wrote: > jdoerfert wrote: > > Consistent style please: > > > > ``` > > if (Value *StrLen =

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value

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

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: cfe-commits, guansong, bollu, hiraditya. Herald added projects: clang, LLVM. As a permanent and

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:200 +OpenMPIRBuilder::InsertPointTy +OpenMPIRBuilder::emitBarrierImpl(const LocationDescription , Directive Kind, +

[PATCH] D70143: Check result of emitStrLen before passing it to CreateGEP

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

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value

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

jdoerfert updated this revision to Diff 229233. jdoerfert added a comment. Adjust paths and test case options Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69922/new/ https://reviews.llvm.org/D69922 Files:

[PATCH] D70289: [OpenMP][NFCI] Use the libFrontend ProcBindKind in Clang

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: cfe-commits, guansong, bollu, jholewinski. Herald added a project: clang. This removes the

[PATCH] D70290: [OpenMP] Use the OpenMPIRBuilder for "omp parallel"

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: guansong, bollu. Herald added a project: clang. NOTE: This patch needs tests and is missing

[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:

[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,

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

jdoerfert marked 3 inline comments as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186 +///{ + +#ifndef OMP_IDENT_FLAG JonChesterfield wrote: > Meinersbur wrote: > > JonChesterfield wrote: > > > jdoerfert wrote: > > > >

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value

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

jdoerfert marked 2 inline comments as done. jdoerfert added inline comments. Comment at: llvm/include/llvm/IR/OpenMPConstants.h:57 +#include "llvm/IR/OpenMPKinds.def" + LLVM_MARK_AS_BITMASK_ENUM(0x7FFF) +}; ABataev wrote: > Why do you use `0x7FFF` as

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

jdoerfert updated this revision to Diff 228772. jdoerfert added a comment. update test case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69922/new/ https://reviews.llvm.org/D69922 Files: clang/include/clang/Basic/LangOptions.def

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

jdoerfert updated this revision to Diff 228792. jdoerfert added a comment. Add cancel_barrier functionality + test, move everything to "Frontend" Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 Files:

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

jdoerfert updated this revision to Diff 228801. jdoerfert added a comment. Use IRBuilder for cancel barriers Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69922/new/ https://reviews.llvm.org/D69922 Files:

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

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMPIRBuilder.cpp:228 + getOrCreateThreadID(getOrCreateIdent(SrcLocStr))}; + bool UseCancelBarrier = !ForceSimpleCall && CancellationBlock; + Value

[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 : LHS) { Nit: `bool RHSIsSubsetOfLHS = isStrictSubset(RHS, LHS)` saves you a line later.

[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/

[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 &) { +FinalizationStack.emplace_back(std::move(FI)); + } ABataev wrote: >

[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 &) { +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] D69922: [OpenMP] Use the OpenMP-IR-Builder

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added a project: clang. This is the initial patch to use the OpenMP-IR-Builder, as discussed on the mailing list ([1]

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

jdoerfert created this revision. jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim. Herald added subscribers: cfe-commits, jfb, guansong, bollu, hiraditya, mgorny. Herald added projects: clang, LLVM. This is

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

jdoerfert marked 5 inline comments as done. jdoerfert added inline comments. Comment at: clang/include/clang/Basic/OpenMPKinds.h:29 - OMPD_unknown + OMPD_unknown, }; These changes do not impact Clang nor do they remove functionality (if OPENMP_DIRECTIVE is

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

jdoerfert updated this revision to Diff 228134. jdoerfert marked 3 inline comments as done. jdoerfert added a comment. LLVM code only, added unit test and fix issues Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/

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

jdoerfert added a comment. In D69785#1737581 , @lebedev.ri wrote: > As far as i can tell the builder does not add any debug info. > Should it? It does now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

jdoerfert updated this revision to Diff 228320. jdoerfert added a comment. Use source location information to build the ident string and debug info. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 Files:

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

jdoerfert updated this revision to Diff 228328. jdoerfert added a comment. Remove accidental newline change Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 Files: llvm/include/llvm/IR/OpenMPConstants.h

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

jdoerfert updated this revision to Diff 228327. jdoerfert added a comment. Adjust RTL attributes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69785/new/ https://reviews.llvm.org/D69785 Files: llvm/include/llvm/IR/DebugInfoMetadata.h

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

jdoerfert updated this revision to Diff 228281. jdoerfert added a comment. Add driver test coverage Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69922/new/ https://reviews.llvm.org/D69922 Files: clang/include/clang/Basic/LangOptions.def

[PATCH] D69952: [OPENMP50]Generalize handling of context matching/scoring.

jdoerfert added inline comments. Comment at: clang/include/clang/Basic/Attr.td:3310 +VariadicUnsignedArgument<"CtxSelectorSets">, +VariadicUnsignedArgument<"CtxSelectors">, VariadicStringArgument<"ImplVendors"> ABataev wrote: > jdoerfert wrote: > >

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

jdoerfert marked 2 inline comments as done. jdoerfert added a comment. In D69853#1737426 , @ABataev wrote: > In D69853#1737319 , @jdoerfert wrote: > > > In D69853#1737264 ,

[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 &) { +FinalizationStack.emplace_back(std::move(FI)); + } jdoerfert

[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

[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

[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] 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:

[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

[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

[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:

[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: >

[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: >

[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

[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 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:

[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

[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

[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 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

[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

[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

[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

[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

[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.

[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

[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

[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

[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

[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

[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

[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/

[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

[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

[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] 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

[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

[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

[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.

[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

[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] 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

[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

[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

[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 =

[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 &&

[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 &&

[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

[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

[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

[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

[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.

[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

[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

<    1   2   3   4   5   6   7   8   9   10   >