[PATCH] D101960: [openmp] Drop requirement on library path environment variables

JonChesterfield added a comment. Just ran into this again. It's really annoying that a test fails, and prints a run line, which can be copied into a terminal where it abjectly fails to run because the environment variables aren't set. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D104904: [OpenMP][AMDGCN] Initial math headers support

JonChesterfield reopened this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Landing ocml side first seems reasonable as it's less likely to be broken and makes testing this more straightforward Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D121466: [OpenMP] Replace math headers with OpenMP wrapper calls

JonChesterfield added a comment. We've got quite a lot of debt in this area and seem at risk of taking on more. Ideally the cuda and hip and openmp headers would be closer to a single header containing: double acosh(double); INSTANTIATE(acosh, cuda_acosh, amdgpu_acosh, intel_acosh);

[PATCH] D121468: [OpenMP] Add linking of OpenMP math library

JonChesterfield added inline comments. Comment at: clang/include/clang/Driver/Options.td:1056 +def libomptarget_nvptx_math_bc_path_EQ : Joined<["--"], "libomptarget-nvptx-math-bc-path=">, + Group, HelpText<"Path to libomptarget-nvptx math bitcode library">; def dD : Flag<["-"]

[PATCH] D121837: [OpenMP][FIX] Allow device constructors for AMD GPU

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

[PATCH] D121837: [OpenMP][FIX] Allow device constructors for AMD GPU

JonChesterfield accepted this revision. JonChesterfield added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121837/new/ https://reviews.llvm.org/D121837 ___ cfe-commits mailing list cf

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. It lets applications run with the libomp and libomptarget built with the toolchain. For users, they don't have to be root. For compiler devs, we test our work instead of whatever the distro had installed. There's no info at that link. What's 'broken rpath'? If t

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a subscriber: estewart08. JonChesterfield added a comment. @estewart08 Fedora are rejecting some uses of rpath (and probably runpath). I can't find a corresponding page for redhat. This may become a problem for our aomp/rocm builds. Repository: rG LLVM Github Monorepo

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. In D118493#3401448 , @tstellar wrote: > The rule that is broken is "standard RPATHs" Fedora installs libomp to > /usr/lib64. > > ... > > I think what we'll do in Fedora is just add -fno-openmp-implicit-rpath to > the d

[PATCH] D122069: [Object] Add binary format for bundling offloading metadata

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Couple of nits above but basically I'm convinced. The gnarly part of binary formats is string tables and I'm delighted that part of MC was readily reusable. Wrapping the stri

[PATCH] D122831: [OpenMP] Make the new offloading driver the default

JonChesterfield added a comment. Editing the tests in place to check for new driver properties would mean we lose coverage for the old driver and get a lot of churn if we need to back out the change. How about taking some of the more interesting driver tests, duplicating them to only run the n

[PATCH] D122831: [OpenMP] Make the new offloading driver the default

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. LGTM, thanks! If it fails CI we might want to submit the tests and the code change separately, but I'm fine with trying it as one block in the first instance. Repository:

[PATCH] D124039: [OpenMP] Add better testing for the linker wrapper

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

[PATCH] D118934: [OpenMP] Completely remove old device runtime

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I've read this closely and can't think of anywhere else that needs to be patched. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D118934: [OpenMP] Completely remove old device runtime

JonChesterfield added inline comments. Comment at: clang/include/clang/Driver/Options.td:2470 Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>; -defm openmp_target_new_runtime: BoolFOption<"openmp-target-new-runtime", - LangOpts<"OpenMPTargetNewRuntime">, DefaultTrue,

[PATCH] D119018: [OpenMP] Add -Bsymbolic to arguments for GNU linker

JonChesterfield added a comment. Can we drop an xfail in an existing test as part of this patch? Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:581 + + // If we are linking for the device all symbols should be bound locally. + if (JA.isDeviceOffloading(Action::OFK_OpenMP)) -

[PATCH] D119018: [OpenMP] Add -Bsymbolic to arguments for GNU linker

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

[PATCH] D119590: exclude openembedded distributions from setting rpath on openmp executables

JonChesterfield added a comment. Cross compilers are a hazard here. I'd expect there to be a fairly long list of magic flags you need to pass to clang to get it to find the right libraries. Can you add fno-openmp-implicit-rpath to that list instead? A better solution might be a cmake flag to sp

[PATCH] D119638: [OpenMP][NFC] Simplify identifying the device bitcode library

JonChesterfield accepted this revision. JonChesterfield added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:2014 - StringRef ArchPrefix = Triple.isAMDGCN() ? "amdgcn" : "nvptx"; - std::string LibOmpTar

[PATCH] D119841: [OpenMP] Pass AMDGPU math libraries into the linker wrapper

JonChesterfield added a comment. I'm not confident it's only attributes that we rely on mlink-builtin-bitcode patching up, that could be poor phrasing on my part. Making the pipeline robust to underspecified IR input may be easier (and arguably more useful) than changing the rocm library to be

[PATCH] D119841: [OpenMP] Pass AMDGPU math libraries into the linker wrapper

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:8205 +if (llvm::find(LibraryArgs, "m") == LibraryArgs.end() && !D.CCCIsCXX()) + continue; + jdoerfert wrote: > JonChesterfield wrote: > > jhuber6 wrote: > > > jdoerf

[PATCH] D119841: [OpenMP] Pass AMDGPU math libraries into the linker wrapper

JonChesterfield added a comment. This is asserting somewhere in clang-linker-wrapper, e.g. https://lab.llvm.org/buildbot/#/builders/193/builds/6939 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119841/new/ https://reviews.llvm.org/D119841 ___

[PATCH] D119841: [OpenMP] Pass AMDGPU math libraries into the linker wrapper

JonChesterfield reopened 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/D119841/new/ https://reviews.llvm.org/D119841 __

[PATCH] D129435: [Clang] Parse toolchain-specific offloading arguments directly

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

[PATCH] D128914: [HIP] Add support for handling HIP in the linker wrapper

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Code looks good to me. It's hard to be sure whether it works without running a bunch of hip test cases through it, have you already done so? If it doesn't work out of the box

[PATCH] D129534: [OpenMP] Do not link static library with `-nogpulib`

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

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

JonChesterfield added a reviewer: b-sumner. JonChesterfield added a comment. Tagging Brian as the code owner of rocm device libs - emitting these in clang would simplify that library. Currently clang reads these commandline flags and conditionally links in bitcode files to introduce these symbo

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

JonChesterfield added a comment. A safer bet is to use the current control flow that links in specific bitcode files, but create the global directly instead of linking in the file. That'll give us zero semantic change and a clang that ignores those bitcode files if present. Repository: rG L

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield created this revision. JonChesterfield added reviewers: arsenm, rampitec, sdesmalen, rjmccall, rnk, aaron.ballman. Herald added subscribers: kosarev, kerbowa, t-tye, Anastasia, tpr, dstuttard, yaxunl, jvesely, kzhuravl. Herald added a project: All. JonChesterfield requested review

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. In D125970#3525985 , @yaxunl wrote: > need a codegen test to make sure amdgpu_kernel ABI is used in C/C++ for > functions with this attribute We've already got tests that check the amdgpu_kernel calling conv is lowered

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. OK, so that's a different thing. CUDA/HIP has a bunch of rules about implicitly tagging things with addrspace(1) at the call boundary. I don't think any of that magic should exist for C or C++, the developer gets to spell out the address space stuff they want ex

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield updated this revision to Diff 430801. JonChesterfield added a comment. - Add O0 arg passing codegen test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125970/new/ https://reviews.llvm.org/D125970 Files: clang/include/clang/Basic/

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. Added a codegen test for arg passing. It establishes that most arguments are left alone, but structs passed by value are handled as an addrspace(4) byref. Letting opt -O2 run annotated some argument pointers as being in addrspace(1) which I think is wrong. I ha

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. Thanks for accepting! I'm interested to learn more about how the calling conv works, e.g. if parts of it are implemented in clang and parts of it patched on the fly by opt, but that's downstream of easy access to writing C tests that use it. Repository: rG L

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield updated this revision to Diff 430818. JonChesterfield added a comment. - Rebase on main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125970/new/ https://reviews.llvm.org/D125970 Files: clang/include/clang/Basic/Attr.td clang/i

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield updated this revision to Diff 430820. JonChesterfield added a comment. - Fix git merge misfires Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125970/new/ https://reviews.llvm.org/D125970 Files: clang/include/clang/Basic/Attr.td

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield updated this revision to Diff 430821. JonChesterfield added a comment. - Fix git merge misfires Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125970/new/ https://reviews.llvm.org/D125970 Files: clang/include/clang/Basic/Attr.td

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. Unintentionally created this patch against an older version of main and it interacted badly with D124998 on the rebase. Rerunning tests now, and will leave this open for further comments for a little while. Thanks all Reposit

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG83c431fb9e72: [amdgpu] Add amdgpu_kernel calling conv attribute to clang (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANG

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. A clangd buildbot (https://lab.llvm.org/buildbot/#/builders/131/builds/27770) failed on this with [ RUN ] SerializationTest.NoCrashOnBadArraySize ==384111==ERROR: ThreadSanitizer failed to allocate 0x1 (65536) bytes of stack depot (error code: 12)

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. If it was adding a calling convention, sure - caution warranted. There's no llvm change here though, an existing CC is exposed to C++. No change to the type system either. I'll propose a patch with some documentation for it if you wish, but it'll just say "For

[PATCH] D125970: [amdgpu] Add amdgpu_kernel calling conv attribute to clang

JonChesterfield added a comment. In D125970#3531645 , @aaron.ballman wrote: > In D125970#3527685 , > @JonChesterfield wrote: > >> If it was adding a calling convention, sure - caution warranted. There's no >> l

[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] D88384: [OpenMP][FIX] Verify compatible types for declare variant calls

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Change looks reasonable in itself too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88384/new/ https://reviews.llvm.org/D88384

[PATCH] D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types

JonChesterfield added a comment. In D85990#2224996 , @erichkeane wrote: > Were you able to make any progress on this? No, interrupted by other things. May have some time later today - only need to work out where the suboptimal diagnostic is emitted and s

[PATCH] D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types

JonChesterfield updated this revision to Diff 286598. JonChesterfield added a comment. - Use more specific diagnostic Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85990/new/ https://reviews.llvm.org/D85990 Files: clang/lib/Sema/SemaDecl.cpp c

[PATCH] D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types

JonChesterfield added a comment. Works out cleanly, thanks for the suggestion. Comment at: clang/lib/Sema/SemaDecl.cpp:12478 if (!Var->isInvalidDecl() && RealDecl->hasAttr()) { + if (Var->getStorageClass() == SC_Extern) { Same as first diff except

[PATCH] D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbcaa806a4747: [Clang] Fix BZ47169, loader_uninitialized on incomplete types (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CH

[PATCH] D85990: [Clang] Fix BZ47169, loader_uninitialized on incomplete types

JonChesterfield added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:34 + +// CHECK: @templ_int = global %struct.templ undef, align 8 +templ templ_int [[clang::loader_uninitialized]]; This broke 32 bit builds where the pointer is

[PATCH] D85735: [OpenMP] Context selector extensions for template functions

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. This looks good to me. I can't claim to have read the test case in detail. Some style suggestions inline around the vector, but they're not blocking. Comme

[PATCH] D85777: [OpenMP] Support std::complex math functions in target regions

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. The duplication from libc++ doesn't feel good but I can see how it happened. Dependency breaking etc. Comment at: clang/lib/Headers/openmp_wrappers/complex

[PATCH] D93079: [OpenMP] Introduce an assumption to ignore possible external callers

JonChesterfield added a comment. Is this for cases where we are compiling a subset of the target code, i.e. without link time optimisation? It's interesting that we might want a static function on the gpu and an external one on the cpu. The user could presumably make it static and provide a di

[PATCH] D85223: [CUDA][HIP] Support accessing static device variable in host code for -fgpu-rdc

JonChesterfield added reviewers: jdoerfert, hfinkel. JonChesterfield added a comment. Herald added a subscriber: dexonsmith. I concede that making the variables external, and trying to give them unique names, does work around static variables not working. I believe static variables are subjected

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield created this revision. JonChesterfield added a reviewer: t-tye. Herald added subscribers: tpr, dstuttard, yaxunl, kzhuravl. JonChesterfield requested review of this revision. Herald added subscribers: cfe-commits, wdng. Herald added a project: clang. [amdgpu] Default to code object

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield updated this revision to Diff 311745. JonChesterfield added a comment. Herald added subscribers: llvm-commits, dang, kerbowa, nhaehnle, jvesely. Herald added a project: LLVM. - update docs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield updated this revision to Diff 311746. JonChesterfield added a comment. - another part of docs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93258/new/ https://reviews.llvm.org/D93258 Files: clang/include/clang/Driver/Options.td

[PATCH] D93258: [amdgpu] Default to code object v3

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4b2e7d021502: [amdgpu] Default to code object v3 (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield added a comment. In D93258#2453724 , @thakis wrote: > reverted in c9ede6f3367a627baeef78f30d18078af9a4ffca > for now Thanks! Just saw the CI emails come through. I didn

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield added a comment. In D93258#2453828 , @yaxunl wrote: > Can they use rocm release branches of llvm/clang with the corresponding rocm > release? As llvm/clang trunk is like the development branch, it is > understandable that they may contain

[PATCH] D93356: [libomptarget][amdgpu] Call into deviceRTL instead of ockl

JonChesterfield created this revision. JonChesterfield added reviewers: jdoerfert, grokos, ABataev, ronlieb, tianshilei1992. Herald added subscribers: t-tye, tpr, dstuttard, yaxunl, jvesely, kzhuravl. JonChesterfield requested review of this revision. Herald added subscribers: openmp-commits, cfe-

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield updated this revision to Diff 312196. JonChesterfield added a comment. - Remove hard coded version numbers in tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93258/new/ https://reviews.llvm.org/D93258 Files: clang/include/cla

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield added a comment. @yaxunl Please see Gerrit 456139 for a close approximation to the test changes here. I don't think hip should be hard coding a version number in tests that don't care about it, so would like to move trunk and internal to a regex. Repository: rG LLVM Github Mo

[PATCH] D93398: [NFC] Use regex for code object version in hip tests

JonChesterfield created this revision. JonChesterfield added a reviewer: yaxunl. JonChesterfield requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. [NFC] Use regex for code object version in hip tests Extracted from D93258

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield added a comment. In D93258#2457905 , @yaxunl wrote: > Thanks for fixing the lit tests. Using regex is the right choice. > > Do we have a plan about how to merge this to amd-stg-open? Will it cause > ePSDB to fail? Do you have a follow up p

[PATCH] D93398: [NFC] Use regex for code object version in hip tests

JonChesterfield added inline comments. Comment at: clang/test/Driver/hip-code-object-version.hip:56 -// V4: "-mllvm" "--amdhsa-code-object-version=4" -// V4: "-targets=host-x86_64-unknown-linux,hip-amdgcn-amd-amdhsa--gfx906" +// VD: "-mllvm" "--amdhsa-code-object-version=4" +//

[PATCH] D93356: [libomptarget][amdgpu] Call into deviceRTL instead of ockl

JonChesterfield planned changes to this revision. JonChesterfield added a comment. There's a codegen test that checks for __ockl_get_local_size. Testing a change to that test out of tree now, probably need to update said test before landing this. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D93398: [NFC] Use regex for code object version in hip tests

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc0619d3b21cd: [NFC] Use regex for code object version in hip tests (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D93258: [amdgpu] Default to code object v3

JonChesterfield updated this revision to Diff 312236. JonChesterfield added a comment. - rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93258/new/ https://reviews.llvm.org/D93258 Files: clang/include/clang/Driver/Options.td clang/lib/Dri

[PATCH] D93258: [amdgpu] Default to code object v3

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdaf39e3f2dba: [amdgpu] Default to code object v3 (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D93079: [OpenMP] Introduce an assumption to ignore possible external callers

JonChesterfield added a comment. Ah, this may be one of the things aomp changed for nvptx. We did something around linking but I've never looked into exactly what. In that case fine by me - it's a sharp edge, but I'd expect it to make a difference in benchmarks until whole program optimisation

[PATCH] D86119: [OPENMP50]Allow overlapping mapping in target constrcuts.

JonChesterfield added a comment. If data mappings can overlap, then it follows that copies to/from the target must be done sequentially by the runtime, unless additional information on their independence exists. Alias analysis style. I see the programmer convenience angle, but it is a shame to

[PATCH] D88829: [OpenMP][RTL] Remove dead code

JonChesterfield added a comment. Change looks great to me. Rolling the reduction in leading whitespace in nvptx_target_parallel_reduction_codegen.cpp in with the patch might be contentious, added a couple more reviewers to see if other people would prefer that part split out. I'll accept in a

[PATCH] D88829: [OpenMP][RTL] Remove dead code

JonChesterfield accepted this revision. JonChesterfield added a comment. Thanks guys, will remember that as the local convention on rolling whitespace changes into other stuff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88829/new/ https://revie

[PATCH] D88929: [OpenMP] Change CMake Configuration to Build for Highest CUDA Architecture by Default

JonChesterfield added a comment. An alternative approach is to build the deviceRTL for multiple cuda versions and then pick whichever one is the best fit when compiling application code. That has advantages when building the deviceRTL libraries on a different machine to the one that intends to

[PATCH] D88929: [OpenMP] Change CMake Configuration to Build for Highest CUDA Architecture by Default

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

[PATCH] D86097: [OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeAMDGCN.cpp:175 + +void CGOpenMPRuntimeAMDGCN::emitSPMDKernelWrapper( +const OMPExecutableDirective &D, StringRef ParentName, The nvptx emitSPMDKernelWrapper does nothing and t

[PATCH] D86097: [OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1344 +FieldDecl *clang::CodeGen::CodeGenUtil::CodeGenUtil::addFieldToRecordDecl( +ASTContext &C, DeclContext *DC, QualType FieldTy) { This appears to be the same as the

[PATCH] D91713: [libomptarget] Implement get_device_num for amdgcn, nvptx

JonChesterfield updated this revision to Diff 307215. JonChesterfield added a comment. Herald added subscribers: llvm-commits, libc-commits, libcxx-commits, lldb-commits, Sanitizers, cfe-commits, mravishankar, teijeong, frasercrmck, dexonsmith, awarzynski, rdzhabarov, ecnelises, tatianashp, wenle

[PATCH] D92167: [OpenMP][NFC] Encapsulate some CGOpenMPRuntime static methods in a utility class

JonChesterfield requested changes to this revision. JonChesterfield added a comment. This revision now requires changes to proceed. This moves some private free functions into a CodeGenUtil class in the header so that the amdgpu plugin can call them in order to construct the kern_desc object. Sa

[PATCH] D86097: [OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN

JonChesterfield requested changes to this revision. JonChesterfield added a comment. This revision now requires changes to proceed. I don't believe the contents of this patch is necessary for codegen on amdgpu. One of the internal/weak distinctions works around a bug in the gfx800 toolchain, but

[PATCH] D86097: [OpenMP][AMDGCN] Generate global variables and attributes for AMDGCN

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp:63 + +llvm::GlobalVariable *CGOpenMPRuntimeNVPTX::allocateTransferMediumGlobal( +CodeGenModule &CGM, llvm::ArrayType *Ty, StringRef TransferMediumName) { Perhaps (ty

[PATCH] D89971: [OpenMP][CUDA] Add missing overload for `remquo(float,float,int*)`

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Change obviously good. Am I right in reading this as all of OvO then passes for trunk, nvptx? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revi

[PATCH] D89994: [libomptarget][nvptx] Undef, internal shared variables

JonChesterfield created this revision. JonChesterfield added reviewers: jdoerfert, ABataev, grokos, tianshilei1992, ye-luo. Herald added a project: clang. Herald added a subscriber: cfe-commits. JonChesterfield requested review of this revision. [libomptarget][nvptx] Undef, internal shared variab

[PATCH] D89994: [libomptarget][nvptx] Undef, internal shared variables

JonChesterfield added a comment. The nvptx back end accepts common + zero + shared, but not common + undef + shared. I think weak_odr is conceptually right here, but given the warning that nvlink doesn't support weak symbols, internal also seems fine. Can someone see an advantage to weak over i

[PATCH] D89994: [libomptarget][nvptx] Undef, internal shared variables

JonChesterfield added a comment. In D89994#2348656 , @ABataev wrote: > In D89994#2348655 , @JonChesterfield > wrote: > >> The nvptx back end accepts common + zero + shared, but not common + undef + >> shared. I th

[PATCH] D89994: [libomptarget][nvptx] Undef, internal shared variables

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:4794 CGM.getModule(), LLVMStaticTy, - /*isConstant=*/false, llvm::GlobalValue::CommonLinkage, - llvm::Constant::getNullValue(LLVMStaticTy), + /*isCons

[PATCH] D89994: [libomptarget][nvptx] Undef, internal shared variables

JonChesterfield updated this revision to Diff 301038. JonChesterfield added a comment. - prefer weak, update tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89994/new/ https://reviews.llvm.org/D89994 Files: clang/lib/CodeGen/CGOpenMPRuntimeG

[PATCH] D89994: [libomptarget][nvptx] Undef, internal shared variables

JonChesterfield abandoned this revision. JonChesterfield added a comment. The diff doesn't look right here. I can't tell if that's a quirk of the phab gui or indicates a bad merge, going to recreate. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89

[PATCH] D90248: [libomptarget][nvptx] Undef, weak shared variables

JonChesterfield created this revision. JonChesterfield added reviewers: jdoerfert, ABataev, grokos, tianshilei1992, ye-luo. Herald added a project: clang. Herald added a subscriber: cfe-commits. JonChesterfield requested review of this revision. [libomptarget][nvptx] Undef, weak shared variables

[PATCH] D90251: [AMDGPU] Add __builtin_amdgcn_grid_size

JonChesterfield created this revision. JonChesterfield added reviewers: yaxunl, arsenm, b-sumner, cfang, rjmccall, Anastasia. Herald added subscribers: openmp-commits, cfe-commits, dexonsmith, kerbowa, t-tye, tpr, dstuttard, nhaehnle, jvesely, kzhuravl. Herald added projects: clang, OpenMP. JonCh

[PATCH] D90251: [AMDGPU] Add __builtin_amdgcn_grid_size

JonChesterfield added a comment. Consistency really. It seemed strange to have a builtin for reading the workgroup size and not one for the grid size. There's probably a range limit that can be set on this one too, I'm just not sure what it is. Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D90248: [libomptarget][nvptx] Undef, weak shared variables

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5d02ca49a294: [libomptarget][nvptx] Undef, weak shared variables (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D90251: [AMDGPU] Add __builtin_amdgcn_grid_size

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdee7704829bd: [AMDGPU] Add __builtin_amdgcn_grid_size (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D90440: [OpenMP][NFC] Clang format ParseOpenMP

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Strongly in favour. Whitespace differencess vs clang format impose a cost to patches, reviews and downstream forks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D95976: [OpenMP] Simplify offloading parallel call codegen

JonChesterfield added inline comments. Comment at: openmp/libomptarget/deviceRTLs/common/generated_microtask_cases.gen:1 +case 0: +((void (*)(kmp_int32 *, kmp_int32 * This is not very pretty. Why do we need runtime dispatch to a function pointer? Repository:

[PATCH] D101976: [OpenMP] Unified entry point for SPMD & generic kernels in the device RTL

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I'm not certain what this 'aligned' limitation for nvptx syncthreads is, but can't think of a corresponding one for amdgcn. So we may not need the LDS barrier construction, a

[PATCH] D101976: [OpenMP] Unified entry point for SPMD & generic kernels in the device RTL

JonChesterfield added inline comments. Comment at: openmp/libomptarget/deviceRTLs/common/src/omptarget.cu:195 + +EXTERN __attribute__((weak)) +int32_t __kmpc_target_init(ident_t *Ident, bool IsSPMD, jdoerfert wrote: > JonChesterfield wrote: > > why are these weak

[PATCH] D102306: Add gfx1034

JonChesterfield added a comment. Is there documentation for the mapping from product names to gfx numbers? It's possible to work it out from roct's topology.cpp and the pci.ids table, but hopefully there's a table we could point users to as well. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D101973: [OpenMP][NFC] Remove SIMD check lines for non-simd tests

JonChesterfield added a comment. I can't see a change to a script in this diff. Should there be one? Looks like the test change was done programmatically Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101973/new/ https://reviews.llvm.org/D101973 _

[PATCH] D95161: [Clang][OpenMP][NVPTX] Replace `libomptarget-nvptx-path` with `libomptarget-nvptx-bc-path`

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

<    1   2   3   4   5   6   7   8   9   >