[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. So you won't articulate or document the new invariant and you think there's a llvm-dev discussion that says we can't verify the invariant which you won't reference, but means you won't add this to the verifier. Request changes doesn't really work after you've ap

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

JonChesterfield added a comment. If the amdgpu backend doesn't require this then it doesn't matter much if other passes undo it. If it's not an invariant, we don't need the verifier to alert people to passes that break it. Git blame on the new code in clang will lead people here where they may

[PATCH] D113538: OpenMP: Start calling setTargetAttributes for generated kernels

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Apologies, I thought I had already accepted this. Thanks for the patch! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113538/new/ https://reviews.llvm.org/D113538 _

[PATCH] D113538: OpenMP: Start calling setTargetAttributes for generated kernels

JonChesterfield added a subscriber: estewart08. JonChesterfield added a comment. Is the behaviour change in the above comments intentional? Pointed out by @estewart08 Comment at: clang/lib/CodeGen/TargetInfo.cpp:9208 - FD->hasAttr(); - if ((IsOpenCLK

[PATCH] D113538: OpenMP: Start calling setTargetAttributes for generated kernels

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9288 + + const bool IsOpenMP = M.getLangOpts().OpenMP && !FD; + if ((IsOpenCLKernel || IsHIPKernel || IsOpenMP) && JonChesterfield wrote: > Here, we do the amdgpu-implicitarg-nu

[PATCH] D113538: OpenMP: Start calling setTargetAttributes for generated kernels

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9288 + + const bool IsOpenMP = M.getLangOpts().OpenMP && !FD; + if ((IsOpenCLKernel || IsHIPKernel || IsOpenMP) && JonChesterfield wrote: > JonChesterfield wrote: > > Here, we d

[PATCH] D114394: Compile-time computation of string attribute hashes

JonChesterfield added inline comments. Comment at: llvm/include/llvm/IR/Attributes.h:79 + bool operator<(AttributeKey const &other) const { +return strcmp(value_, other.value_) < 0; + } Could order by size first here, then strncmp on equal sizes Repositor

[PATCH] D114865: [AMDGPU][OpenMP] Use -amdgpu-fixed-function-abi

JonChesterfield accepted this revision. JonChesterfield added a comment. This would unblock a patch that has been waiting for us since May (D102107 ). Once your patches land and make the flag unnecessary we can take it back out. I asked for this because I need t

[PATCH] D114865: [AMDGPU][OpenMP] Use -amdgpu-fixed-function-abi

JonChesterfield added a comment. Fixed abi was set to true in D96340 . At that point, indirect calls probably worked. This was replaced with some dubious fine grain attribute handling in D99347 , at which point indirect calls are

[PATCH] D114865: [AMDGPU][OpenMP] Use -amdgpu-fixed-function-abi

JonChesterfield added a comment. Run a bunch of tests locally. This patch or something equivalent is a precondition on using the new device runtime on amdgpu, which we are very much out of time on (see D114890 which will break us as soon as it lands without th

[PATCH] D114890: [OpenMP] Make the new device runtime the default

JonChesterfield added a comment. D114891 enables this for the amdgpu tests. This patch will leave the nvptx tests running on the new runtime twice, and not on the old runtime at all, I think. lit.cfg should probably use old + new explicitly, instead of default

[PATCH] D114890: [OpenMP] Make the new device runtime the default

JonChesterfield added a comment. In D114890#3164899 , @tianshilei1992 wrote: > Do we still want to run tests for the old device runtime? Maybe? We definitely don't want to run the tests for the new one twice Repository: rG LLVM Github Monorepo CHAN

[PATCH] D102107: [OpenMP] Codegen aggregate for outlined function captures

JonChesterfield added a comment. This works approximately as well as trunk does for me, provided D114865 is also applied. My baseline is not totally solid but I think there's a credible chance this would pass the buildbot, provided D114865

[PATCH] D114890: [OpenMP] Make the new device runtime the default

JonChesterfield added a comment. This will definitely break amdgpu bot without D114865 landed first. However that patch is currently blocked by Matt, so we may want to land this and disable the amdgpu buildbot until the backend is fixed. Repository: rG LLV

[PATCH] D114890: [OpenMP] Make the new device runtime the default

JonChesterfield accepted this revision. JonChesterfield added a comment. The consensus internally seems to be to land this and see what happens on the amdgpu buildbot. Go for it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114890/new/ https://rev

[PATCH] D114890: [OpenMP] Make the new device runtime the default

JonChesterfield added a comment. Fails on CI pretty much like it fails locally https://lab.llvm.org/buildbot/#/builders/193/builds/2569 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114890/new/ https://reviews.llvm.org/D114890

[PATCH] D114865: [AMDGPU][OpenMP] Use -amdgpu-fixed-function-abi

JonChesterfield added a comment. Probably obsoleted by 729bf9b26b657df8 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114865/new/ https://reviews.llvm.org/D114865 _

[PATCH] D131639: [OpenMP] Remove 'stdbool.h' from OpenMP header wrappers

JonChesterfield added a comment. We should write down (maybe in the commit message) what this is fixing. The linked bug has someone defining bool as a workaround for msvc which shouldn't be applied when not compiling with msvc, so superficially it looks like this patch works around broken appli

[PATCH] D82087: AMDGPU/clang: Add builtins for llvm.amdgcn.ballot

JonChesterfield added a comment. In D82087#3803464 , @arsenm wrote: > In D82087#3797883 , @jdoerfert wrote: > >> Can we land this? I'd like to use the new intrinsics as I don't understand >> the old ones. > > What

[PATCH] D135076: [Clang] Make offloading flags accept '-' and '--'

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Like that a lot, good quality of life improvement. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135076/new/ https://reviews.l

[PATCH] D130096: [Clang][AMDGPU] Emit AMDGPU library control constants in clang

JonChesterfield added a comment. In D130096#3816149 , @arsenm wrote: > I'd prefer to avoid spreading special treatment of the device libraries into > the backend. The contract is poorly defined and spread around too much as it > is

[PATCH] D140226: [NVPTX] Introduce attribute to mark kernels without a language mode

JonChesterfield added a comment. Calling convention is the right model here. Kernels are functions with a different calling convention to the 'normal' functions in a very literal sense. The calling convention modelling in clang is different to attribute handling and changing nvptx to it is prob

[PATCH] D141105: [OpenMP] Add support for '--offload-arch=native' to OpenMP offloading

JonChesterfield added a comment. Possible naming hazard here. march=native means target the local processor architecture, zen2 or whatever, and we have the host CPU as an offloading target already. So what I'd expect this to do is host offloading with the openmp runtime compiled for the local v

[PATCH] D141440: [OpenMP] Adjust phases for AMDGPU offloading for OpenMP in save-temps mode

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. If i'm reading this right, the change means we emit the same save-temps files as hip with the same naming convention. That sounds great, makes life easier for backend devs lo

[PATCH] D137524: clang/AMDGPU: Emit atomicrmw for atomic_inc/dec builtins

JonChesterfield added a comment. I don't recognize atomicrmw udec_wrap and can't find it in https://llvm.org/docs/LangRef.html#atomicrmw-instruction. I do vaguely recall the semantics of these builtins (well, the instructions they target) being surprising, Do you know where the uinc_wrao etc we

[PATCH] D143306: [Driver] Default to -fno-openmp-implicit-rpath

JonChesterfield added reviewers: tianshilei1992, ye-luo, RaviNarayanaswamy. JonChesterfield added a comment. Added some people I can remember from the original discussion. The effect of this patch will be that clang -fopenmp foo.cpp will build an executable that doesn't know where its runtime li

[PATCH] D141859: [amdgpu-arch] Dynamically load the HSA runtime if not found during the build

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Yes, ok. Not thrilled about the copy&paste from openmp but we can fix that as soon as we agree on a subdir to put code shared between llvm/clang/openmp and that has been tric

[PATCH] D142133: [LinkerWrapper] Use `clang` to perform the device linking

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Herald added a subscriber: ormris. That's much cleaner, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142133/new/ http

[PATCH] D142138: [OpenMP] Clean up AMD handling for `-fopenmp-targets=amdgcn` arch inference

JonChesterfield added a subscriber: arsenm. JonChesterfield added a comment. @arsenm as above, mcpu != march important? llc takes a different one to clang iirc Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142138/new/ https://reviews.llvm.org/D142

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. That works if you have one toolchain installed globally or you are happy to cobble together a working system using environment variables. If you have multiple toolchains, they can't all sit in the global directory. If you don't have root, you can't install them

[PATCH] D127246: [LinkerWrapper] Rework the linker wrapper and use owning binaries

JonChesterfield added a comment. I've read it but can't promise it's correct - the diff is large and has some spurious noise in it which distracts significantly from the functional changes. Would you be willing to split this into two patches, one which renames variables and moves blocks of cod

[PATCH] D127246: [LinkerWrapper] Rework the linker wrapper and use owning binaries

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I think this is probably OK. Smaller patches usually get reviewed faster so minimising the line noise in the browser is worthwhile. Comment at: clang/tools

[PATCH] D155986: [clang][AMDGPU]: Don't use byval for struct arguments in function ABI

JonChesterfield added a comment. Thanks! Happy to see function calls getting cheaper Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155986/new/ https://reviews.llvm.org/D155986 ___ cfe-commits mailing lis

[PATCH] D157738: [OpenMP] Emit offloading entries for indirect target variables

JonChesterfield added a comment. > calling device functions via their associated host pointer What does this mean? Defining a function foo such that the host and each individual target each have their own machine code for it, such that &foo can be copied over to the target and then invoked to m

[PATCH] D157738: [OpenMP] Emit offloading entries for indirect target variables

JonChesterfield added a comment. If calling an indirect function pointer on the GPU requires a table lookup (keyed by host function addresses, which I didn't think we knew at GPU compile time), and we cannot distinguish indirect function pointers from function pointers, then this feature must s

[PATCH] D157738: [OpenMP] Emit offloading entries for indirect target variables

JonChesterfield added a comment. Ok, I'm really sure this needs to be reflected in the type system. &foo for some target foo gets to be larger than 8 bytes and we use a different calling convention for it. Otherwise however we carve this the type erasure is going to make unrelated calls acquire

[PATCH] D158246: [amdgpu] WIP variadics

JonChesterfield created this revision. Herald added subscribers: libc-commits, foad, kerbowa, hiraditya, tpr, dstuttard, yaxunl, jvesely, kzhuravl, arsenm. Herald added projects: libc-project, All. JonChesterfield requested review of this revision. Herald added subscribers: llvm-commits, cfe-commi

[PATCH] D158246: [amdgpu] WIP variadics

JonChesterfield added a comment. The lowering pass is broadly right, missing a few edge cases. Comment at: libc/config/gpu/entrypoints.txt:88 +libc.src.stdio.vsnprintf libc.src.stdio.puts libc.src.stdio.fopen ^these try to build, but fail. I haven

[PATCH] D158246: [amdgpu] WIP variadics

JonChesterfield added inline comments. Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:159 +// conventions. Escape analysis on va_list values. +return false; + } Cut this from the WIP patch as the draft is noisy. Ran some tests through x64 with a pa

[PATCH] D158246: [amdgpu] WIP variadics

JonChesterfield added a comment. @arsenm suggested va_start/va_end should be addrspace aware, similar idea to https://reviews.llvm.org/D15. Should be less invasive for the va intrinsics as they're not used so widely. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D158246: [amdgpu] WIP variadics

JonChesterfield updated this revision to Diff 551616. JonChesterfield added a comment. - Rename ExpandVAIntrinsics to DesugarVariadics Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158246/new/ https://reviews.llvm.org/D158246 Files: clang/lib/Co

[PATCH] D158246: [amdgpu] WIP variadics

JonChesterfield removed subscribers: kristof.beyls, wangpc, jdoerfert. JonChesterfield added inline comments. Comment at: llvm/lib/CodeGen/ExpandVAIntrinsics.cpp:215-217 +auto alloced = Builder.Insert( +new AllocaInst(VarargsTy, DL.getAllocaAddrSpace(), nullptr, +

[PATCH] D158246: [amdgpu] WIP variadics

JonChesterfield added a comment. Name candidates - expand - lower - desugar - transform Lowering probably makes the most sense for the abi level apply to all functions, I like desugar to cover rewriting a subset of the graph Comment at: libc/config/gpu/entrypoints.txt:84-85

[PATCH] D156928: [Clang][AMDGPU] Fix handling of -mcode-object-version=none arg

JonChesterfield added a comment. What does code objects version= none mean? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156928/new/ https://reviews.llvm.org/D156928 ___ cfe-commits mailing list cfe-com

[PATCH] D156928: [Clang][AMDGPU] Fix handling of -mcode-object-version=none arg

JonChesterfield added a comment. In D156928#4561849 , @arsenm wrote: > In D156928#4561811 , > @JonChesterfield wrote: > >> What does code objects version= none mean? > > Handle any version So... That should be t

[PATCH] D156928: [Clang][AMDGPU] Fix handling of -mcode-object-version=none arg

JonChesterfield added a comment. Or, the front end could define those objects directly, without importing IR files that define the objects with the content clang used to choose the object file. E.g. instead of the argument daz=off (spelled differently) finding a file called daz.off.ll that defi

[PATCH] D156928: [Clang][AMDGPU] Fix handling of -mcode-object-version=none arg

JonChesterfield added a comment. In D156928#4562412 , @yaxunl wrote: > Some control variables are per-module. Clang cannot emit control variables > that have different values for different modules. Intrinsics should work > since it can take an argument

[PATCH] D154036: [libc] Add support for creating wrapper headers for offloading in clang

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. OK, let's go with this. It's a fairly alarming mess localised quite closely to the language that requires the complexity, minimal damage to libc itself. Repository: rG LLV

[PATCH] D153725: [clang] Make amdgpu-arch tool work on Windows

JonChesterfield added a comment. The right thing to do on Linux for this is to query the driver directly. That is, the kernel should populate some string under /sys that we read. That isn't yet implemented. Does windows happen to have that functionality available? (landed here while trying to w

[PATCH] D153725: [clang] Make amdgpu-arch tool work on Windows

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/AMDGPUArch.cpp:50 +#else + return printGPUsByHSA(); +#endif yaxunl wrote: > jhuber6 wrote: > > arsenm wrote: > > > The HIP path should work on linux too. I generally think we should build

[PATCH] D153725: [clang] Make amdgpu-arch tool work on Windows

JonChesterfield added a comment. In D153725#4484747 , @arsenm wrote: > In D153725#4484711 , > @JonChesterfield wrote: > >> The right thing to do on Linux for this is to query the driver directly. >> That is, the

[PATCH] D153725: [clang] Make amdgpu-arch tool work on Windows

JonChesterfield added a comment. In D153725#4484966 , @arsenm wrote: > In D153725#4484754 , > @JonChesterfield wrote: > >> - if you open the driver too many times at once it fails to open, so running >> a parall

[PATCH] D153725: [clang] Make amdgpu-arch tool work on Windows

JonChesterfield added a comment. The test detection is an awkward compromise between people who want to run the GPU tests and people who don't, and reflects diverse hardware in use and variation on whether cuda / hsa are installed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D154850: [libc] Remove GPU string functions incompatible with C++

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Is this a const thing? If so I'd naively hope we can declare both in C++ mode and alias them, but I can believe that is detectably broken. How does glibc manage their hack?

[PATCH] D142569: [OpenMP] Introduce kernel environment

JonChesterfield added a comment. How does a function find the corresponding kernel environment at runtime? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142569/new/ https://reviews.llvm.org/D142569 ___ c

[PATCH] D155191: clang/HIP: Directly use f32 exp and log builtins

JonChesterfield accepted this revision. JonChesterfield added a comment. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155191/new/ https://reviews.llvm.org/D155191 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[PATCH] D159118: [libc] Implement the 'clock()' function on the GPU

JonChesterfield added a comment. I think something needs to notice when clock_t is a different thing on the host and on the GPU for the offloading languages and complain, probably fatally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159118/new/

[PATCH] D138473: clang/HIP: Inline frexp/frexpf implementations

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Noisy diff! Getting rid of the openmp special case handling here is nice. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138473/new/ https://reviews.llvm.org/D138473

[PATCH] D138391: clang/HIP: Add new header test for math IR gen

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Good call adding this test, thanks. I think some of the skipping is glibc quirk related and some was to avoid clobbering the user's namespace for openmp. The math header defi

[PATCH] D138862: [Clang] Do not set offload kind in a freestanding build

JonChesterfield added a comment. Does this change the annotation on kernels compiled from C or C++ with -ffreestanding? If so, probably want a test showing the change. I have a vague idea that they pick up a default of 'opencl' at present, where 'none' is probably better. Or perhaps this is onl

[PATCH] D138392: clang/HIP: Fix broken implementations of __make_mantissa* functions

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Patch is obviously fine. Given these are internal functions, perhaps we should annotate them with the non-null attribute and delete the early exit. CHANGES SINCE LAST ACTION

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

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

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

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

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

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

[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: openmp/libomptarget/deviceRTLs/common/src/par

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

[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. `_Z3foov.ompva

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

<    1   2   3   4   5   6   7   8   9   >