[PATCH] D62046: [OpenMP][bugfix] Add missing math functions variants for log and abs.

JonChesterfield added a comment. I'm not sure about this diff. I think it's breaking and . Raised bug https://bugs.llvm.org/show_bug.cgi?id=42972 Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62046/new/ https://reviews.llvm.org/D62046

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#173 , @ABataev wrote: > In D64943#158 , @JonChesterfield > wrote: > > > > OpenMP linker script is known to cause problems for gold and lld linkers > > > on Linux

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. > OpenMP linker script is known to cause problems for gold and lld linkers on > Linux and it will also cause problems for Windows enabling in future What are the known problems with the linker script? I'm wondering if they can be resolved without the overhead

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#179 , @ABataev wrote: > In D64943#178 , @JonChesterfield > wrote: > > > In D64943#173 , @ABataev wrote: > > > > > In

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#1666849 , @sdmitriev wrote: > In D64943#136 , @JonChesterfield > wrote: > > > I'm not sure copying the crtbegin/crtend mechanism from the early days of C > > runtime

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. I'm not sure copying the crtbegin/crtend mechanism from the early days of C runtime is ideal. Since the data is stored in a common section anyway, please could we rename it to __omp_offloading_entries in which case the linker will provide start/end symbols

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. > Hm, I was not aware of this Linux linker feature, thanks a lot for the > explanation! I see only one problem with using it as a replacement for the > begin/end objects – it looks like `__start_name`/`__stop_name` symbols are > created with `default`

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. I'm on board with getting rid of the linker script. Gold's limited support for that seems conclusive. I believe the current script does two things: 1/ takes a binary and embeds it in a section named .omp_offloading.amdgcn-amd-amdhsa 2/ provides start/end

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. > I see some problems with using llvm-objcopy for that. First issue is that > symbols created by llvm-objcopy for embedded data depend on the input file > name. As you know these symbols are referenced from the offload registration > code that is currently

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. >> Does this diff mix getting rid of the linker script with other changes? E.g. >> it looks like the metadata generation is moving from clang to the new tool, >> but that seems orthogonal to dropping the linker script. > > Metadata is still generated by the

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

JonChesterfield added a comment. I still don't understand what advantage the standalone tool has over renaming the file to `omp_offloading` and then using `objcopy -I binary`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68166/new/

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

JonChesterfield added a comment. I think this patch is a behaviour change. Currently, the target binary is embedded in the host binary at link time. With this change, the contents of the binary are embedded in bitcode which is subsequently fed into the link. If indeed so, that seems strictly

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

JonChesterfield added inline comments. Comment at: clang/include/clang/Driver/Options.td:1643 HelpText<"Emit OpenMP code only for SIMD-based constructs.">; +def fopenmp_new_codegen : Flag<["-"], "fopenmp-new-codegen">, Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>,

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

JonChesterfield added inline comments. Comment at: clang/test/OpenMP/barrier_codegen.cpp:22 +// CLANGCG-NOT: readonly +// IRBUILDER: ; Function Attrs: nofree nosync nounwind readonly +// IRBUILDER-NEXT: declare i32 @__kmpc_global_thread_num(%struct.ident_t*)

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

JonChesterfield added inline comments. Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186 +///{ + +#ifndef OMP_IDENT_FLAG jdoerfert wrote: > Meinersbur wrote: > > jdoerfert wrote: > > > JonChesterfield wrote: > > > > Sharing constants between the compiler and

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

JonChesterfield added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011 unsigned NamedModifiersNumber = 0; - SmallVector FoundNameModifiers( - OMPD_unknown + 1); + SmallVector + FoundNameModifiers(unsigned(OMPD_unknown) + 1); I wonder if

[PATCH] D69498: IR: Invert convergent attribute handling

JonChesterfield added a comment. Requiring the presence of an attribute for correctness is a bad thing. OpenMP was missing this annotation in a number of places and is probably still missing it elsewhere. I wouldn't bet on handwritten bitcode libraries getting it right everywhere either. An

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

JonChesterfield added a comment. Uncertainty over the handling of constant data between clang and libopenmp not withstanding, I think this is good to go. Comment at: llvm/include/llvm/IR/OpenMPKinds.def:186 +///{ + +#ifndef OMP_IDENT_FLAG Meinersbur wrote: >

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

JonChesterfield added a comment. Great to see the fragile math.h stuff disappear. I'm not sure about the CPU/GPU/other granularity. An openmp program with x86 as the host and target offload regions for amdgcn and for nvptx seems like a reasonable aspiration. Or for a couple of different

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

JonChesterfield added inline comments. Comment at: clang/lib/Headers/__clang_cuda_cmath.h:70 __DEVICE__ float fmod(float __x, float __y) { return ::fmodf(__x, __y); } -// TODO: remove when variant is supported -#ifndef _OPENMP jdoerfert wrote: > As far as I can

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

JonChesterfield added inline comments. Comment at: clang/test/OpenMP/begin_declare_variant_codegen.cpp:71 +} + +// Make sure all ompvariant functions return 1 and all others return 0. The name mangling should probably append the device kind, .e.g.

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

JonChesterfield added a comment. In D71241#1782846 , @ABataev wrote: > But I suggest to discuss this with Richard Smith. Is the appeal to authority necessary to resolve this? The last few posts by Hal look clear to me. Especially convincing is: >

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

JonChesterfield added a comment. Lowering in sema or in codegen seems a standard phase ordering choice. There will be pros and cons to both. I think prior art leans towards sema. Variants are loosely equivalent to tag dispatching or constexpr if, both handled before lowering the AST to IR.

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

JonChesterfield added a comment. Has anyone actually asked Richard to look at this? He isn't subscribed to the diff and may not be watching openmp-dev. I don't think it's reasonable to stall progress on optimising openmp indefinitely. Richard may find it difficult to find time to resolve this.

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

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Nice cleanup, thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70289/new/ https://reviews.llvm.org/D70289

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

JonChesterfield added a comment. > Explain that you're replacing the function written by the user on the fly by > another one. If they accept it, go ahead. That's the observational effect of variants. Replacing is very similar to calling + inlining. Repository: rG LLVM Github Monorepo

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

JonChesterfield added a comment. In D71241#1782427 , @ABataev wrote: > In D71241#1782425 , @JonChesterfield > wrote: > > > > Explain that you're replacing the function written by the user on the fly > > > by

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

JonChesterfield added a comment. > 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 the original source. That's pretty convincing. Repository: rG LLVM Github Monorepo

[PATCH] D71103: [libomptarget][nfc] Move three more files to common

JonChesterfield updated this revision to Diff 232557. JonChesterfield added a comment. - update comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71103/new/ https://reviews.llvm.org/D71103 Files:

[PATCH] D69494: OpenMP: Add helper function for convergent runtime calls

JonChesterfield added a reviewer: grokos. JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69494/new/ https://reviews.llvm.org/D69494

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

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

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

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. In D69785#1763317 , @jdoerfert wrote: > I'm confused. Was this a review? I'm waiting for a decision here so we can > move on and

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

JonChesterfield added a comment. 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 infra before the users are landed so the unit test is particularly appreciated. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. In D64943#1682452 , @hfinkel wrote: > This LGTM. I'm happy that this is a design improvement over the current > scheme. @JonChesterfield , @ABataev , any further comments? This patch mixes two concerns. 1/ Remove the

[PATCH] D64943: [Clang][OpenMP offload] Eliminate use of OpenMP linker script

JonChesterfield added a comment. The three way split looks great, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64943/new/ https://reviews.llvm.org/D64943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. The direction is good and I believe all the feedback from D64943 has already been incorporated. LGTM, thanks. CHANGES SINCE LAST ACTION

[PATCH] D68166: [Clang][OpenMP Offload] Add new tool for wrapping offload device binaries

JonChesterfield added a comment. > The tool indeed does not have anything specific to OpenMP at this step, but > that will change... That makes sense to me, thanks. I think we're going to have some trouble adapting this to our build as there's already a standalone tool that runs at link time.

[PATCH] D71830: [OpenMP] Reusable OpenMP context/traits handling

JonChesterfield added a comment. Big patch but looks like a net decrease in complexity. Please could you clang format the areas phabricator is complaining about? Reading through on a browser looks great. I'll take a closer look in a real editor once Christmas is out of the way. Thanks for

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

JonChesterfield created this revision. JonChesterfield added reviewers: kcc, rjmccall, rsmith, glider, vitalybuka, pcc, eugenis, vlad.tsyrklevich, jdoerfert, gregrodgers. Herald added a project: clang. Herald added a subscriber: cfe-commits. [Clang] Uninitialize attribute on global variables

[PATCH] D74513: [OpenMP][NFCI] Use the libFrontend DefaultKind in Clang

JonChesterfield added a comment. Nice. Thank you Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74513/new/ https://reviews.llvm.org/D74513 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

JonChesterfield added a comment. Procedural note - adding someone as a blocking reviewer to someone else's patch isn't great. What if the new reviewer never gets around to looking at the patch? I've adjusted that to non-blocking, but feel free to leave a comment if I've missed something.

[PATCH] D74571: [OpenMP][CUDA] Add CUDA 10.2 support

JonChesterfield added a comment. Do the in tree tests all pass with the 10.2 toolchain? That's not exactly the same as whether it works but is the closest approximation we have available. Assuming yes, this patch seems uncontroversial. CHANGES SINCE LAST ACTION

[PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

JonChesterfield added a comment. I'd like to rebase this on the current deviceRTL, and add any nvptx/amdgcn specific hooks if necessary. Any objections? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59319/new/ https://reviews.llvm.org/D59319

[PATCH] D74925: [OpenMP][Opt] Combine `struct ident_t*` during deduplication

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Nice. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74925/new/ https://reviews.llvm.org/D74925

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. The above patch composes sensibly with openmp, e.g. #include #pragma omp declare target int data __attribute__((no_zero_initializer)); #pragma omp allocate(data) allocator(omp_pteam_mem_alloc) #pragma omp end declare target @data = hidden

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 247617. JonChesterfield added a comment. - Rename attribute, propose some documentation Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 247652. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Address review comments, more test coverage Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. Fixed the spelling/formatting, added more tests. The C++ case would be improved by warning on `int x __attribute__((loader_uninitialised)) = 0` as there are two initializers. The semantics for C are not what I

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. I've continued thinking about / experimenting with this. Curiously - `X x;` and `X x{};` are considered distinct by clang. The current patch will only accept the former. I'll add some tests for that. I think there's a reasonable chance that the developers who

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 247745. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Reduce scope to trivial default constructed types Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield marked 4 inline comments as done. JonChesterfield added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4367 +This is useful for variables that are always written to before use where the +default zero initialization provided by the toolchain

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 247156. JonChesterfield added a comment. - clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files: clang/include/clang/Basic/Attr.td

[PATCH] D75285: Mark restrict pointer or reference to const as invariant

JonChesterfield added a comment. In D75285#1897247 , @yaxunl wrote: > If users derive a non-const pointer from the const pointer and modify it, > doesn't that result in UB? Thanks. No. Modifying a const object is UB, so e.g. we can segv if it's in

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield marked an inline comment as done. JonChesterfield added inline comments. Comment at: clang/test/CodeGenCXX/attr-no-zero-initializer.cpp:40 +// CHECK: @unt = global %struct.nontrivial undef +nontrivial unt [[clang::no_zero_initializer]];

[PATCH] D74361: [Clang] Undef attribute for global variables

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

[PATCH] D74941: [OpenMP] `omp begin/end declare variant` - part 1, parsing

JonChesterfield added a subscriber: gregrodgers. JonChesterfield added a comment. Thanks! Splitting this out of D71179 , which I think ultimately reached consensus, makes the diff much easier to read. Subscribing Greg, as I think this is a path to removing a lot

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 247753. JonChesterfield added a comment. - error on extern variables, minor cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 247776. JonChesterfield added a comment. - Error on redeclaration with loader_uninit in C Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. I'm finally happy with the semantic checks here. Thanks for the guidance on where to look for the hooks. - attributed variable must be at global scope - all initializers are rejected - default constructors must be trivial (to reduce the scope of this patch) -

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 247696. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Reject initialisers, update doc to recommended string Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield marked 2 inline comments as done. JonChesterfield added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; +

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

JonChesterfield updated this revision to Diff 247127. JonChesterfield added a comment. - Rename attribute, add to hasDefiningAttr Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files:

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. In D74361#1879559 , @rjmccall wrote: > This will need to impact static analysis and the AST, I think. Local > variables can't be redeclared, but global variables can, so disallowing > initializers syntactically when the

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield marked an inline comment as done. JonChesterfield added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6509 +static void handleNoZeroInitializerAttr(Sema , Decl *D, const ParsedAttr ) { + D->addAttr(::new (S.Context)

[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

JonChesterfield added a comment. Patch looks good with the above nits. I'm not totally sure about the callback vs running a separate IR pass after the finalize() call, but when the callback is this simple it looks fine. I like that this preserves the current semantics.

[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

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

[PATCH] D74571: [OpenMP][CUDA] Add CUDA 10.2 support

JonChesterfield added a comment. @tra that's great context, thank you very much for writing it up. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74571/new/ https://reviews.llvm.org/D74571 ___ cfe-commits mailing list

[PATCH] D74571: [OpenMP][CUDA] Add CUDA 10.2 support

JonChesterfield added a comment. Interesting distinction. Should compiling without warning indicate comprehensive support, or merely that we ran the tests and they passed? I assumed the latter on the basis that we probably don't have comprehensive support for cuda 10.1 either. No preference

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

JonChesterfield added a comment. It zero initialises on x86, but perhaps by coincidence rather than guarantee. Fair enough regarding reuse of the existing attribute, I'll create a new one. Thanks for the pointers on additional cases to test for too. I'll return with an improved patch.

[PATCH] D71948: [OpenMP] Use the OpenMPIRBuilder for `cancel` directives

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I like this, thanks. Very clear. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:241 + // This seems to be used only once without much change of

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added a comment. In D75917#1916160 , @sameerds wrote: > how this builtin fits in with the overall scheme of language-specific and > target-specific details of an atomic operation. For example, is this meant > only for OpenCL? Does it

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. ping @aaron.ballman - does that look right to you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 ___ cfe-commits mailing

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 248807. JonChesterfield marked 2 inline comments as done. JonChesterfield added a comment. - Review comments, add tests for private_extern Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. > I thought err_loader_uninitialized_extern says that the variable cannot have > external linkage? Embarrassing! This was a badly written error message, now fixed to: `external declaration of variable cannot have the 'loader_uninitialized' attribute` The

[PATCH] D73979: [HIP] Allow non-incomplete array type for extern shared var

JonChesterfield added a comment. In D73979#1857736 , @yaxunl wrote: > BTW this is requested by HIP users, who have similar code for CUDA and HIP. > They found it surprised that nvcc allows it but hip-clang does not. I think I'm one of the HIP users

[PATCH] D75788: [WIP][OpenMP] Reuse CUDA wrappers in `nvptx` target regions.

JonChesterfield added a comment. That's less invasive than I feared. Nicely done. It may worth keeping the openmp header wrapper to do architecture dispatch. Something like: #ifndef __CLANG_OPENMP_MATH_DECLARES_H__ #define __CLANG_OPENMP_MATH_DECLARES_H__ #ifndef _OPENMP #error

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3713 + switch (ord) { + case 0: // memory_order_relaxed + default: // invalid order Interesting, fence can't be relaxed > ‘fence’ instructions take an ordering

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:1516 +// Builtin to expose llvm fence instruction +BUILTIN(__builtin_memory_fence, "vUiUi", "t") `BUILTIN(__builtin_memory_fence, "vii", "n")`? The other fence

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added a subscriber: jdoerfert. JonChesterfield added a comment. @jdoerfert this is one of the two intrinsics needed to drop the .ll source from the amdgcn deviceRTL. The other is atomic_inc. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added a comment. In D75917#1916972 , @sameerds wrote: > Well, there is a problem: The LangRef says that scopes are target-defined. > This change says that scopes are defined by the high-level language and > further assumes that OpenCL

[PATCH] D77774: [OpenMP] Allow to go first in C++-mode in target regions

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. The cmath/math.h story makes me sad, but this is a good workaround. Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added a comment. The tests look good, but I can't see the implementation in this diff. Maybe a file missing from the patch? Can be hard to tell with phabricator, the error may be at my end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13651 + llvm::getConstantStringInfo(Scope, scp); + SSID = getLLVMContext().getOrInsertSyncScopeID(scp); + sameerds wrote: > saiislam wrote: > > sameerds wrote: > > >

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9 + // CHECK: fence syncscope("workgroup") seq_cst + __builtin_memory_fence(__ATOMIC_SEQ_CST, "workgroup"); + JonChesterfield wrote: > sameerds wrote: > >

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:1 // REQUIRES: amdgpu-registered-target // RUN: %clang_cc1 %s -x hip -emit-llvm -O0 -o - \ Codegen test should be under CodeGen and/or CodeGenCXX Repository:

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/test/CodeGenHIP/builtin_memory_fence.cpp:9 + // CHECK: fence syncscope("workgroup") seq_cst + __builtin_memory_fence(__ATOMIC_SEQ_CST, "workgroup"); + sameerds wrote: > JonChesterfield wrote: > >

[PATCH] D77918: [OpenMP] Avoid crash in preparation for diagnose of unsupported type

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

[PATCH] D77390: Fix __builtin_amdgcn_workgroup_size_x/y/z return type

JonChesterfield added inline comments. Comment at: clang/test/CodeGenOpenCL/builtins-amdgcn.cl:541 switch (d) { - case 0: *out = __builtin_amdgcn_workgroup_size_x(); break; + case 0: *out = __builtin_amdgcn_workgroup_size_x() + 1; break; case 1: *out

[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

JonChesterfield abandoned this revision. JonChesterfield added a comment. No problem. This isn't on the live path - the function is mostly called from openmp codegen and clang doesn't target openmp/amdgcn just yet. I'll roll this change into the codegen patch to enable that. Repository: rG

[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

JonChesterfield added a comment. In D78495#1992404 , @arsenm wrote: > Needs test? I'm not sure how to write said test. How do we normally hit asserts from the clang test suite? This fires a lot in the openmp on amdgcn downstream branch, but I'm happy

[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

JonChesterfield created this revision. JonChesterfield added reviewers: rjmccall, aaron.ballman, ABataev, jdoerfert, arsenm. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. [nfc] Accept addrspacecast allocas in InitTempAlloca Changes the precondition to be slightly

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 250263. JonChesterfield added a comment. - Amend diagnostic as suggested, clang-format new lines in SemaKinds.td Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. In D74361#1920329 , @aaron.ballman wrote: > Aside from the diagnostic wording, I think this LG to me. However, I'd > appreciate if @rjmccall would also sign off. Thanks! @rjmccall? Comment at:

[PATCH] D74361: [Clang] Undef attribute for global variables

JonChesterfield updated this revision to Diff 250264. JonChesterfield marked an inline comment as done. JonChesterfield added a comment. - undo reformat of existing def Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/

[PATCH] D76342: [OpenMP] Implement '#pragma omp tile'

JonChesterfield added a comment. edit: actually you've already done the clang-format on trunk as I hoped, phabricator mislead me. Apologies for the noise Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76342/new/ https://reviews.llvm.org/D76342

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added a comment. Can this be revived? Changing the enum to a string still sounds good to me Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 ___ cfe-commits

[PATCH] D77113: [OpenMP][NFC] Move and simplify directive -> allowed clause mapping

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. LGTM too. Non functional change, clearer code. Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77113/new/

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1888 +// Check valididty of memory ordering as per C11 / C++11's memody model. +if (ord < static_cast(llvm::AtomicOrderingCABI::acquire) || + ord >

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added a comment. Amdgcn specific is fine by me. Hopefully that unblocks this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75917/new/ https://reviews.llvm.org/D75917 ___ cfe-commits

[PATCH] D75917: Expose llvm fence instruction as clang intrinsic

JonChesterfield added inline comments. Comment at: clang/test/CodeGenCXX/builtin-amdgcn-fence-failure.cpp:5 + +void test_amdgcn_fence_failure() { + arsenm wrote: > Does this really depend on C++? Can it use OpenCL like the other builtin > tests?This also

  1   2   3   4   5   6   7   8   9   >