[PATCH] D84476: Make hip math headers easier to use from C

JonChesterfield created this revision. JonChesterfield added a reviewer: yaxunl. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. Make hip math headers easier to use from C Motivation is a step towards using the hip math headers t

[PATCH] D83832: [OpenMP] Provide a flag to disable safety checks for GPU optimizations

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I think this is good. It's dangerous, but it's also undocumented and has unsafe in the name. I should be able to use this to sidestep limitations in the amdgpu function poin

[PATCH] D83832: [OpenMP] Provide a flag to disable safety checks for GPU optimizations

JonChesterfield added a comment. I think there's an unfortunate interaction with link time optimisation here. If there are external regions, but their code is combined with llvm-link before codegen, then a user could reasonably assume this flag is safe. Would it would be correct to compile the

[PATCH] D83281: [OpenMP] Allow traits for the OpenMP context selector `isa`

JonChesterfield added a comment. Agreed on tests. I like the mechanism - passing a string through to the backend as a way to dispatch between isa properties looks cleanly extensible. We probably do want to emit a warning when the backend claims it doesn't know anything about said string as it'l

[PATCH] D83723: [OpenMP] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:39 + // return constant compile-time target-specific warp size + unsigned WarpSize = CGF.getTarget().getGridValue(llvm::omp::GV_Warp_Size); + return Bld.getInt32(WarpSize); -

[PATCH] D83723: [OpenMP] Generalize CGOpenMPRuntimeNVPTX as CGOpenMPRuntimeGPU

JonChesterfield added a comment. This is indeed the direction I had in mind. Broad strokes look right. I probably wouldn't notice an accidental change amidst the whitespace reshuffle. Very happy to read through line by line if you can split the whitespace change out. Comment

[PATCH] D83591: [OpenMP][CUDA] Fix std::complex in GPU regions

JonChesterfield added a comment. In D83591#2145437 , @jdoerfert wrote: > I did not know they are using __clang_cuda headers. (Site note, we should > rename them then.) I also did not know that. I am repeatedly caught out by things named 'cuda', 'nvptx'

[PATCH] D83591: [OpenMP][CUDA] Fix std::complex in GPU regions

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Fine by me. Let's get nvptx working properly in tree now and work out how to wire up amdgcn subsequently. I'm sure a reasonable abstraction will present itself. Repository:

[PATCH] D83492: [OpenMP] Use common interface to access GPU Grid Values

JonChesterfield added a comment. Changing to getGridValue would be useful for sharing parts of this with amdgcn. The aomp toolchain handles codegen for amdgcn by adding if (isAMDGCN) to this file. Until such time as tregions obsoletes this code, I think we should go with layers instead of scatt

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

JonChesterfield added a comment. __kmpc_spmd_kernel_init is always called with RequiresDataSharing == 0 Specifically, it's only called from clang, and emitSPMDEntryHeader unconditionally passes zero to it I.e. I think there's more stuff that can be cleaned up in the theme of the above, suggest

[PATCH] D83349: [OpenMP][NFC] Remove unused and untested code from the device runtime

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Applied to the amdgcn implementation. Compiles fine, tests all passing. Seems likely that this lot really is dead. Interesting that this removes*_data_sharing_environment. I

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

JonChesterfield added a comment. In D83268#2135938 , @ABataev wrote: > > @Hahnfeld @ABataev - are you sufficiently persuaded that preserving the > > current interface is not worth the development cost? > > No, I'm not. Long before that, we relied on the A

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Aside from the API stability concern this looks uncontentious. Removes dead arguments, generally makes things simpler. Thus LGTM. @Hahnfeld @ABataev - are you sufficiently pe

[PATCH] D83268: [OpenMP][NFC] Remove unused (always fixed) arguments

JonChesterfield added a comment. I'm not sure we have a consensus on api stability. Usually llvm allows mixing libraries and compilers from different sources, e.g. the various libunwind or compiler-rt vs libgcc. Libomptarget in general appears to be considered fixed and has external users (int

[PATCH] D80858: [CUDA][HIP] Support accessing static device variable in host code

JonChesterfield added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:10068 +isa(D) && cast(D)->isFileVarDecl() && +cast(D)->getStorageClass() == SC_Static) { + return GVA_StrongExternal; yaxunl wrote: > rjmccall wrote: > > Are you

[PATCH] D83121: [AMDGPU] Change Clang AMDGCN atomic inc/dec builtins to take unsigned values

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

[PATCH] D80897: [OpenMP] Initial support for std::complex in target regions

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I think this change is good. The library story is a bit difficult, but fundamentally openmp needs a shim of some sort to map target math functions onto the libm of the underl

[PATCH] D80804: [AMDGPU] Introduce Clang builtins to be mapped to AMDGCN atomic inc/dec intrinsics

JonChesterfield added a comment. This patch declares the clang builtin as acting on signed values, but the IR intrinsic maps to an instruction which does an unsigned comparison. We don't have ISA support for a signed comparison equivalent. Addition is the same operation on signed or unsigned in

[PATCH] D78759: Add Statically Linked Libraries

JonChesterfield added a comment. This appears to have been committed without addressing all the comments or waiting for an acceptance from someone outside of our organisation. That doesn't seem right - am I missing part of the thread here? Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D80858: [HIP] Support accessing static device variable in host code

JonChesterfield added a comment. The value is based on llvm::sys::Process::GetRandomNumber(). So unless one provides a build-system-derived uuid for every compilation unit, recompiling identical source will yield an observably different binary. The distinction between 'unique' and 'random' is s

[PATCH] D80804: [AMDGPU] Expose llvm atomic inc/dec instructions as clang builtins for AMDGPU target

JonChesterfield added a comment. Thanks for this. I like the refactor to share code with amdgcn_fence. Agreed with Matt's points above. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80804/new/ https://reviews.llvm.org/D80804 ___

[PATCH] D46472: [HIP] Support offloading by linker script

JonChesterfield added inline comments. Herald added a reviewer: jdoerfert. Comment at: cfe/trunk/lib/Driver/ToolChains/CommonArgs.cpp:1329 + // Get the HIP offload tool chain. + auto *HIPTC = static_cast( + C.getSingleOffloadToolChain()); Should this be `t

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

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

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

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

[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 https://reviews.llvm.

[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] 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 https://reviews.llvm.org/D4/

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

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

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

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

[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 > static_cast(llvm::AtomicOrderingCABI

[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/ https://reviews

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

[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] D74361: [Clang] Undef attribute for global variables

JonChesterfield added a comment. In D74361#1927863 , @thakis wrote: > This breaks tests on Windows: http://45.33.8.238/win/10664/step_7.txt > > Please take a look, and if it takes some time please revert while you > investigate. Thanks! It seems Windows

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

JonChesterfield added a comment. Tests made assumptions about alignment which are invalid on arm, failed a buildbot. The tests don't actually care about alignment, so fixed by dropping the `, align #` part of the patterns. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGc45eaeabb77a: [Clang] Undef attribute for global variables (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://r

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

JonChesterfield added a comment. Thanks guys. Patched the C codegen test before landing - minor change on trunk in the last week, orthogonal to this. Delighted to have one less reason to write assembly. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

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

JonChesterfield updated this revision to Diff 250901. JonChesterfield added a comment. - Update C codegen test to match output of trunk Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 Files: clang/include

[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 https://lists.llvm

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

JonChesterfield added a comment. In D75917#1925617 , @sameerds wrote: > Is it really? The scope argument of the IR fence is a target-specific string: > http://llvm.org/docs/LangRef.html#syncscope > > The change that I see here is assuming a numerical argu

[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/ https://reviews.llvm.o

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

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

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

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

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

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

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

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

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

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

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

[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 https://lists.llvm

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

[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: clang/include

[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: clang/include/clang

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

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

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

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

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

[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: clang/include/c

[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 addrspace(3

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

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

[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 clang/lib/A

[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 &S, Decl *D, const ParsedAttr &AL) { + D->addAttr(::new (S.Context) NoZeroInitializerAttr(S.Co

[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] 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: clang/include/clang

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

[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 cfe-commits@lists.llvm.

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

[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 https://reviews.llvm.org/D7457

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

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

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

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

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

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

[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. 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] 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: > We're

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

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

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

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

[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

<    3   4   5   6   7   8   9   >