[PATCH] D109885: [MLIR][[amdgpu-arch]][OpenMP] Remove direct dependency on /opt/rocm

JonChesterfield added a comment. @ronlieb do you have any more information? The patch says remove /opt/rocm but the discussion seems to be going another way. What's the actual req here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109885/new/ htt

[PATCH] D109885: [MLIR][[amdgpu-arch]][OpenMP] Remove direct dependency on /opt/rocm

JonChesterfield added a comment. Sounds good to me. > CMAKE_INSTALL_PREFIX should be removed. I think we have consensus on that ^, orthogonal to the /opt/rocm presence or absence. As long as there's some way to tell cmake to use a given HSA, and it seems there are several, deleting that string

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

JonChesterfield created this revision. JonChesterfield added reviewers: yaxunl, t-tye, kzhuravl, ronlieb, b-sumner. Herald added subscribers: kerbowa, pengfei, tpr, dstuttard, nhaehnle, jvesely. JonChesterfield requested review of this revision. Herald added subscribers: cfe-commits, sstefan1, wdng

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

JonChesterfield added a comment. Thanks. I'm going to wait for some of the rocm people to pass judgement too as this code path is shared with hip / opencl etc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98746/new/ https://reviews.llvm.org/D9874

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124 + // Currently defaults to 3 in AMDGPUBaseInfo.cpp + // Using that default lets clang emit IR for amdgcn when llvm has been built + // without that target, provided the user want

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124 + // Currently defaults to 3 in AMDGPUBaseInfo.cpp + // Using that default lets clang emit IR for amdgcn when llvm has been built + // without that target, provided the user want

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

JonChesterfield added a comment. This appears to have missed a case for openmp. Credit to @jdoerfert for the repro: https://godbolt.org/z/xWTYbv struct DST { long X; long Y; }; #pragma omp declare target DST G2 [[clang::loader_uninitialized, clang::address_space(5)]]; #prag

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

JonChesterfield added a comment. Great context, thanks guys. I had missed that part of the compiler. We presently have a dependency edge from clang to the amdgcn target in llvm. The drawback I noticed here is it stops clang unconditionally building a runtime library for amdgcn. This patch, or a

[PATCH] D99402: [AMDGPU][OpenMP] Add /include to the search path

JonChesterfield added a comment. The general problem looks harder but important to fix. Finding the right headers but the wrong shared library is bad, and iirc we currently have to use LD_LIBRARY_PATH to bodge the latter which is not a good UX. Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D99506: [OpenMP][NFC] Move the `noinline` to the parallel entry point

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Context in https://bugs.llvm.org/show_bug.cgi?id=49752 is that this resolves a regression in stack usage from D94315 . This change looks goo

[PATCH] D99297: [OPENMP]Fix PR49636: Assertion `(!Entry.getAddress() || Entry.getAddress() == Addr) && "Resetting with the new address."' failed.

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

[PATCH] D99447: [OpenMP] Define omp_is_initial_device() variants in omp.h

JonChesterfield added a comment. Nice, thanks! I think the function in the devicertl is dead with this change, maybe remove that too? Comment at: openmp/runtime/src/include/omp.h.var:479 +# endif + # undef __KAI_KMPC_CONVENTION jdoerfert wrote: > hbae wro

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. This change is partly motivated by wanting to check in runtime tests for openmp that execute on whatever hardware is available locally. It is functionally similar to an out of tree bash script called mygpu that contains manually curated tables of pci.ids and to

[PATCH] D99447: [OpenMP] Define omp_is_initial_device() variants in omp.h

JonChesterfield accepted this revision. JonChesterfield added a comment. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99447/new/ https://reviews.llvm.org/D99447 ___ cfe-commits mailing lis

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. I'm happy with this as-is. @jdoerfert is this close enough to what you expected when we discussed this offline? Adding it directly to AMDGPU.h was a good suggestion. Makes it easy for other amdgpu language drivers to pick it up. That it's a binary on disk also

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. We'll have slightly indirect testing once this is used to enable D99656 . There are two pieces that can be tested: 1/ The clang handling. That we can test with a minor change. Add a command line argument to clang that specifies t

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/include/clang/Driver/Options.td:921 HelpText<"HIP runtime installation path, used for finding HIP version and adding HIP include path.">; +def amdgpu_arch_tool_path_EQ : Joined<["--"], "amdgpu-arch-tool-path=">, Group,

[PATCH] D100144: [AMDGPU] Allow relaxed/consume memory order for atomic inc/dec

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Nice. Thank you for fixing the oversight. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100144/new/ https://reviews.llvm.org/D100144 ___

[PATCH] D100124: [Clang][NVPTX] Add NVPTX intrinsics and builtins for CUDA PTX redux.sync instructions

JonChesterfield added a comment. Interesting. Reduction across lanes in warp? If so, this is probably a way to handle the last step reduction for openmp reductions CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100124/new/ https://reviews.llvm.org/D100124 __

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. New tests look good to me, thanks. Comment at: clang/include/clang/Driver/Options.td:922 +def amdgpu_arch_tool_EQ : Joined<["--"], "amdgpu-arch-tool=">, Group, + HelpText<"Path to amdgpu_arch tool, used for detecting AMD GPU arch in the system

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. In D99949#2688869 , @gregrodgers wrote: > 1. It does not provide the infrastructure to identify runtime capabilities to > satisfy requirements of a compiled image. I believe we only require a value for '-march=' to unbloc

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. In D99949#2689583 , @gregrodgers wrote: > Dependence on hsa is not necessary. The amdgpu and nvidia drivers both use > PCI codes available in /sys . We should use architecture independent methods > as much as possible.

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield accepted this revision. JonChesterfield added a comment. I've built this, checked it behaves as expected, checked clang does something reasonable when the executable is missing. All looks good to me, explicitly accepting. We may need an internal call with Greg to work out how to

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) gregrodgers wrote: > What happens when /opt/rocm

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) JonChesterfield wrote: > gregrodgers wrote: > >

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:9 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS /opt/rocm) +if (NOT ${hsa-runtime64_FOUND}) t-tye wrote: > JonChesterfield wrote: > > JonChe

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. The failing log pointed at `"-mllvm" "--amdhsa-code-object-version=4"`, which is a hard error if the amdgpu triple is missing from the llvm. I see the test features amdgpu-registered-target. Perhaps that does not behave as one might wish? I'd suggest rebuilding

[PATCH] D100620: [OpenMP] Make sure classes work on the device as they do on the host

JonChesterfield added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:20 +// need to `include `. +#include +#endif I think there are legitimate use cases for writing code that doesn't include new. Can we add the

[PATCH] D69498: IR: Invert convergent attribute handling

JonChesterfield added a subscriber: sameerds. JonChesterfield added a comment. Herald added subscribers: Naghasan, lxfind, okura, bbn, kuter, kerbowa, pengfei, jrtc27. Herald added a reviewer: sstefan1. Herald added a reviewer: baziotis. I've managed to run a bug in some freestanding c++ compiled

[PATCH] D100620: [OpenMP] Make sure classes work on the device as they do on the host

JonChesterfield added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:20 +// need to `include `. +#include +#endif jdoerfert wrote: > JonChesterfield wrote: > > I think there are legitimate use cases for writing

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. Couple of minor points above. I think the increase in error reporting granularity will be helpful if this falls over in the field, as well as helping if we need a third try to get through PPC CI Comment at: clang/lib/Driver/ToolChains/AMDGPUOp

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. LG, thank you for the debugging, and for the more descriptive failure reporting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D9

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

JonChesterfield added a comment. That is a great feature, thank you. Compiling state machines and scheme programs to C is now much prettier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99517/new/ https://reviews.llvm.org/D99517

[PATCH] D100929: [Clang] Allow the combination of loader_uninitialized and address spaces

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I misread this as implying loaded_uninitialized for addrspace globals, but actually it's a straightforward oversight from the original implementation. Thanks!

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

JonChesterfield added a comment. I was under the impression that `#!/usr/bin/env sh` is a sensible invocation for running a shell on various systems. The current theory for this struggling with the ppc buildbot is that fedora doesn't support that. Ad hoc searching suggests 'sh' is required to e

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

JonChesterfield updated this revision to Diff 314379. JonChesterfield added a comment. - update test, fix whitespace Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93356/new/ https://reviews.llvm.org/D93356 Files: clang/lib/CodeGen/CGOpenMPRuntim

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG76bfbb74d38b: [libomptarget][amdgpu] Call into deviceRTL instead of ockl (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANG

[PATCH] D94060: [OpenMP][AMDGPU] Use AMDGPU_KERNEL calling convention for entry function

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. LGTM. This is the same hook hip & opencl use to indicate that a function is a kernel entry point. We may need to create the metadata openmp nvptx uses as well, but that's sep

[PATCH] D38968: [OpenMP] Implement omp_is_initial_device() as builtin

JonChesterfield added subscribers: jdoerfert, JonChesterfield. JonChesterfield added a comment. Herald added subscribers: sstefan1, guansong, yaxunl. Herald added a reviewer: jdoerfert. @jdoerfert suggested replacing this with a context selector. One less special case in clang. Related, if this

[PATCH] D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment

JonChesterfield added a comment. I'm guessing we're using the function boundary as a compiler barrier. That seems fragile in the face of improving cross-function optimisation. What is the invariant we want around entry to a data environment, which happens to be met by a function boundary? Rep

[PATCH] D94961: [OpenMP] Add OpenMP offloading toolchain skeleton for AMDGPU

JonChesterfield added a comment. This patch was written, roughly, by: - copying the known-working openmp driver from rocm into the trunk source tree - deleting lots of stuff that didn't look necessary - deleting some stuff that is broadly necessary, but the specifics are up for debate The idea

[PATCH] D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR

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

[PATCH] D94884: [Clang][OpenMP] Include header for CUDA builtin vars into OpenMP wrapper header

JonChesterfield added a comment. I don't think introducing everything from the cuda namespace into openmp nvptx offloading is a feature. Inevitably people will call threadIdx.x instead of the openmp or clang equivalent, and this will mask missing functionality in openmp. I won't object too stro

[PATCH] D95007: [CUDA][HIP] Add -fuse-cuid

JonChesterfield added subscribers: jdoerfert, JonChesterfield. JonChesterfield added a reviewer: jdoerfert. JonChesterfield added a comment. @jdoerfert this may be a candidate for deriving names of extracted target regions CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95007/new/ https:

[PATCH] D95007: [CUDA][HIP] Add -fuse-cuid

JonChesterfield added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2307 + // Only alphanumeric and underscore is allowed in -cuid option. + if (auto *A = Args.getLastArg(OPT_cuid_EQ)) { +const char *V = A->getValue(); Why this limi

[PATCH] D111218: [AMDGPU][OpenMP] Mark oulined functions always_inline

JonChesterfield edited reviewers, added: tianshilei1992, ye-luo, grokos; removed: ggeorgakoudis. JonChesterfield added a comment. Not pretty but unblocking D102107 is important. Could you write up your current understanding of what we're miscompiling for funct

[PATCH] D111218: [AMDGPU][OpenMP] Remove optnone from outlined functions

JonChesterfield added a comment. Root cause would definitely be better but we don't have one at present and would like to unblock the linked diff. O0 is likely to have more problems than just this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111

[PATCH] D105191: [Clang][OpenMP] Add partial support for Static Device Libraries

JonChesterfield added a comment. Blocking windows + osx for a couple of hours is bad, let's revert it. Will do so myself when I get to a desktop Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105191/new/ https://reviews.llvm.org/D105191 __

[PATCH] D111311: [Clang][OpenMP] Fix fat archive tests for Mac and Windows

JonChesterfield accepted this revision. JonChesterfield added a comment. Looks plausible to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111311/new/ https://reviews.llvm.org/D111311 ___ cfe-commits m

[PATCH] D110286: [WIP][Clang][OpenMP] Add new clang argument `-fopenmp-target-simd`

JonChesterfield added a comment. I'm excited about the thread to warp mapping but unclear why this needs to be a compile time flag. Can we use this mapping when pragma simd is present and otherwise stay with the current one? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D111475: [OpenMP] Add RTL function for getting number of threads in block.

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Great! I like the decrease in complexity of the compiler. Pretty close to dropping the nvptx and amdgpu subclasses entirely Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D111993: [libomptarget][WIP] Patch amdgpu DeviceRTL until it compiles

JonChesterfield created this revision. JonChesterfield added a reviewer: jdoerfert. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl. JonChesterfield requested review of this revision. Herald added subscribers: openmp-commits, cfe-commits, sstefan1, wdn

[PATCH] D111993: [libomptarget][WIP] Patch amdgpu DeviceRTL until it compiles

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:255 options::OPT_fno_openmp_target_new_runtime, false)) -BitcodeSuffix = "new-amdgcn-" + GPUArch; +BitcodeSuffix = "new-amdgpu-" + GPUArch; else --

[PATCH] D111993: [libomptarget][WIP] Patch amdgpu DeviceRTL until it compiles

JonChesterfield added inline comments. Comment at: openmp/libomptarget/DeviceRTL/src/Synchronization.cpp:87 +#if 0 +// Can't spell the dispatch from runtime ordering like: +template Suggestion offline is that delaying the check in clang for these builtins until

[PATCH] D111993: [libomptarget][WIP] Patch amdgpu DeviceRTL until it compiles

JonChesterfield planned changes to this revision. JonChesterfield added a comment. Needs manual rebase on trunk + clang patch to delay checking the arguments to amdgcn intrinsics until template instantiation time Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield created this revision. JonChesterfield added reviewers: gribozavr, rsmith, ABataev, dblaikie, aaron.ballman, jdoerfert. JonChesterfield requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fixes a compiler assert on passing a co

[PATCH] D111993: [libomptarget][WIP] Patch amdgpu DeviceRTL until it compiles

JonChesterfield added inline comments. Comment at: openmp/libomptarget/DeviceRTL/src/Synchronization.cpp:92 +} +// clang/lib/AST/ExprConstant.cpp:14739: bool clang::Expr::EvaluateAsInt(clang::Expr::EvalResult&, const clang::ASTContext&, clang::Expr::SideEffectsKind, bool) const

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield added a comment. In D112159#3075703 , @jdoerfert wrote: > This makes sense, any reason we don't remove the check at the call sites > right away? There are ~85 of them, will start looking through now. Repository: rG LLVM Github Monore

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield updated this revision to Diff 381037. JonChesterfield added a comment. - fix up call sites Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112159/new/ https://reviews.llvm.org/D112159 Files: clang/lib/AST/ExprConstant.cpp clang/t

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield updated this revision to Diff 381039. JonChesterfield added a comment. - fix up call sites Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112159/new/ https://reviews.llvm.org/D112159 Files: clang/lib/AST/ExprConstant.cpp clang/l

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield updated this revision to Diff 381040. JonChesterfield added a comment. - drop spurious nl Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112159/new/ https://reviews.llvm.org/D112159 Files: clang/lib/AST/ExprConstant.cpp clang/li

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield added a comment. Patched up the call sites that are straightforward. There are ~4 calls in SemaOpenMP where an `!isValueDependent()` could be dropped if it is safe to call PerformContextualImplicitConversion on such a value, which it probably is, but it's hard to be certain. The

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:2137 // // Let's see if this is a constant < 0. If so, we reject it out of hand, // per CWG1464. Otherwise, if it's not a constant, we must have an Shame about the wh

[PATCH] D112159: Relax assert in ExprConstant to a return None.

JonChesterfield added a comment. Thanks! Lots of call sites to check, appreciate the check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112159/new/ https://reviews.llvm.org/D112159 ___ cfe-commits maili

[PATCH] D112159: Relax assert in ExprConstant to a return None.

This revision was automatically updated to reflect the committed changes. Closed by commit rG7ff4f48adb26: Relax assert in ExprConstant to a return None. (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112159/new/ https:

[PATCH] D111993: [libomptarget][WIP] Patch amdgpu DeviceRTL until it compiles

JonChesterfield abandoned this revision. JonChesterfield added a comment. Going to discard this and recreate as a new patch which includes the two cmake ones (D111983 and D111987 ) Repository: rG LLVM Github Monorepo CHANG

[PATCH] D112227: [libomptarget] Build DeviceRTL for amdgpu

JonChesterfield created this revision. Herald added subscribers: kerbowa, t-tye, tpr, dstuttard, yaxunl, mgorny, nhaehnle, jvesely, kzhuravl. JonChesterfield requested review of this revision. Herald added subscribers: openmp-commits, cfe-commits, sstefan1, wdng. Herald added a reviewer: jdoerfert

[PATCH] D112227: [libomptarget] Build DeviceRTL for amdgpu

JonChesterfield added inline comments. Comment at: openmp/libomptarget/DeviceRTL/src/Configuration.cpp:23 -extern uint32_t __omp_rtl_debug_kind; +// extern uint32_t __omp_rtl_debug_kind; Otherwise the missing symbols prevents linking, not clear why it works o

[PATCH] D112227: [libomptarget] Build DeviceRTL for amdgpu

JonChesterfield added inline comments. Comment at: openmp/libomptarget/DeviceRTL/src/Configuration.cpp:23 -extern uint32_t __omp_rtl_debug_kind; +// extern uint32_t __omp_rtl_debug_kind; jdoerfert wrote: > JonChesterfield wrote: > > Otherwise the missing symb

[PATCH] D112227: [libomptarget] Build DeviceRTL for amdgpu

JonChesterfield added subscribers: ronlieb, pdhaliwal, carlo.bertolli, gregrodgers, dpalermo. JonChesterfield added a comment. Subscribed some AMD people to this. I wanted to apply this patch as-is to amd-stg-open to feed it to the internal testing, but it doesn't apply because Driver/ToolChain

[PATCH] D108380: [openmp][nfc] Refactor GridValues

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2a47a84b4011: [openmp][nfc] Refactor GridValues (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D108380: [openmp][nfc] Refactor GridValues

JonChesterfield added a comment. Failed a nvptx codegen test (maybe the change to calculate log2 at runtime), currently away from my desk but will revert when I get back unless beaten to it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108380/new/

[PATCH] D108552: [OpenMP][AMDGCN] Enable complex functions

JonChesterfield added a comment. Macros fine by me, haven't found time to look at what is going on with variant Comment at: clang/lib/Headers/openmp_wrappers/complex:20 +#ifdef __NVPTX__ #define __OPENMP_NVPTX__ I don't think this should be necessary - the d

[PATCH] D108380: [openmp][nfc] Refactor GridValues

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:551 + llvm::Log2_32(CGF.getTarget().getGridValue().GV_Warp_Size); + unsigned LaneIDMask = ~0 >> (32u - LaneIDBits); auto &RT = static_cast(CGF.CGM.getOpenMPRuntime()); ---

[PATCH] D108380: [openmp][nfc] Refactor GridValues

JonChesterfield updated this revision to Diff 368100. JonChesterfield added a comment. - require unsigned shift Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108380/new/ https://reviews.llvm.org/D108380 Files: clang/include/clang/Basic/TargetInf

[PATCH] D108380: [openmp][nfc] Refactor GridValues

This revision was automatically updated to reflect the committed changes. Closed by commit rGc2574e63ff71: [openmp][nfc] Refactor GridValues (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108380/new/ https://reviews.llv

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

JonChesterfield added a comment. Pasting `env LD_LIBRARY_PATH=` and `env LIBRARY_PATH` into the printed commandline, as opposed to through magic, would solve most of my day to day frustration here. libomp.so and libomptarget.so are in two different directories, LD_LIBRARY_PATH turns out to be c

[PATCH] D101935: [clang] Search runtimes build tree for openmp runtime

JonChesterfield added a comment. At present libomptarget_amdgcn_bc_path and nvptx need to be a path to a file. If we relax that to accept a path to a directory in which the corresponding file is found, then we can use that argument in place of LIBRARY_PATH from the test scripts. That will let t

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

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:648 +void tools::addOpenMPRuntimeSpecificRPath(const ToolChain &TC, + const ArgList &Args, JonChesterfield wrote: > protze.joa

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

JonChesterfield added a comment. In D101960#2960657 , @jdoerfert wrote: > Do I understand correctly that adding runpath to libomp.so will help it find > libomptarget.so *and* still allows users to use LD_LIBRARY_PATH to make sure > a different libomptar

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

JonChesterfield added a comment. In D101960#2960846 , @protze.joachim wrote: > The lit config has platform-specific rules to build the environmental > variables (including the use of the proper separators). I suggest to used > these values to create th

[PATCH] D108708: [openmp][amdgpu] Initial gfx10 offloading implementation

JonChesterfield created this revision. JonChesterfield added reviewers: dpalermo, jdoerfert, tianshilei1992, ronlieb, ye-luo, grokos. Herald added subscribers: kerbowa, guansong, t-tye, tpr, dstuttard, yaxunl, mgorny, nhaehnle, jvesely, kzhuravl. JonChesterfield requested review of this revision.

[PATCH] D108708: [openmp][amdgpu] Initial gfx10 offloading implementation

JonChesterfield added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h:102 + +template constexpr const GV &getAMDGPUGridValues() { + static_assert(wavesize == 32 || wavesize == 64, ""); Want to resolve this at compile time for the

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

JonChesterfield added a comment. Test setup can be fixed independently (and possibly should be). D102043 is newly simplified. It looks for plugins next to libomptarget.so, which means it can find them even when the application uses a non-transitive method to f

[PATCH] D108774: [OpenMP][FIX] Allow declare variant to work with reference types

JonChesterfield added a comment. That is a surprising root cause. It turns out std::cexp does in fact take the complex argument by const reference. The C versions take it by value. Shall we guard this with if (openmp) or similar to avoid it changing semantics for other languages? Repository:

[PATCH] D108708: [openmp][amdgpu] Initial gfx10 offloading implementation

JonChesterfield updated this revision to Diff 369067. JonChesterfield added a comment. - uint32 to unsigned Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108708/new/ https://reviews.llvm.org/D108708 Files: clang/lib/Basic/Targets/AMDGPU.h llvm

[PATCH] D108708: [openmp][amdgpu] Initial gfx10 offloading implementation

JonChesterfield updated this revision to Diff 369069. JonChesterfield added a comment. - add 1030 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108708/new/ https://reviews.llvm.org/D108708 Files: clang/lib/Basic/Targets/AMDGPU.h llvm/include/l

[PATCH] D108708: [openmp][amdgpu] Initial gfx10 offloading implementation

JonChesterfield marked an inline comment as done. JonChesterfield added a subscriber: dhruvachak. JonChesterfield added inline comments. Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:111 +set(mcpus gfx700 gfx701 gfx801 gfx803 gfx900 gfx902 gfx906 gfx908 gfx1010

[PATCH] D108708: [openmp][amdgpu] Initial gfx10 offloading implementation

JonChesterfield added inline comments. Comment at: openmp/libomptarget/plugins/amdgpu/src/rtl.cpp:1921 DP("Max_Teams: %d\n", Max_Teams); -DP("RTLDeviceInfoTy::Warp_Size: %d\n", RTLDeviceInfoTy::Warp_Size); +DP("RTLDeviceInfoTy::Warp_Size: %d\n", WarpSize); DP("R

[PATCH] D108708: [openmp][amdgpu] Initial gfx10 offloading implementation

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG78f92c38101f: [openmp][amdgpu] Initial gfx10 offloading implementation (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D101935: [clang] Search runtimes build tree for openmp runtime

JonChesterfield planned changes to this revision. JonChesterfield added a comment. Yeah, we probably can't leave the installed program with a search path only used for testing. I'm planning to revisit this to use libomptarget_amdgcn_bc_path instead of LIBRARY_PATH for testing, then drop the LIB

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

JonChesterfield created this revision. JonChesterfield added reviewers: tianshilei1992, grokos, jdoerfert. Herald added subscribers: guansong, yaxunl. JonChesterfield requested review of this revision. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. The commandline

[PATCH] D109061: [openmp] No longer use LIBRARY_PATH to find devicertl

JonChesterfield created this revision. JonChesterfield added reviewers: jdoerfert, grokos, tianshilei1992. Herald added subscribers: guansong, yaxunl. JonChesterfield requested review of this revision. Herald added subscribers: openmp-commits, cfe-commits, sstefan1. Herald added projects: clang, Op

[PATCH] D109061: [openmp] No longer use LIBRARY_PATH to find devicertl

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1701 StringRef ArchPrefix = Triple.isAMDGCN() ? "amdgcn" : "nvptx"; + std::string LibOmpTargetName = "libomptarget-" + BitcodeSuffix.str() + ".bc"; This part is co

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

JonChesterfield added a comment. In D109057#2977084 , @jdoerfert wrote: > LG, but please add a test for this. D109061 sends everything in check-openmp down this code path, but we should indeed have a check for the file

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

JonChesterfield updated this revision to Diff 369969. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109057/new/ https://reviews.llvm.org/D109057 Files: clang/test/Driver/openmp-offload-gpu.c Index: clang/test/Driver/openmp-offload-gpu.c

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

JonChesterfield added a comment. Well, that's not helpful. Managed to add a test through the webui as arcanist was misbehaving but it has worked by deleting the functional change. Will try again Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109057

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

JonChesterfield updated this revision to Diff 369971. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109057/new/ https://reviews.llvm.org/D109057 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp clang/test/Driver/openmp-offload-gpu.c Index: cl

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

JonChesterfield updated this revision to Diff 369972. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109057/new/ https://reviews.llvm.org/D109057 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp clang/test/Driver/openmp-offload-gpu.c Index: cl

[PATCH] D109057: [openmp] Accept directory for libomptarget-bc-path

JonChesterfield added a comment. OK, test added, same code as originally, checked that the test fails on trunk. Because it's driver only we don't get a far as the fatal error, but you can see the driver passing the directory through to builtin-bitcode. Will land when the premerge checks catch u

<    3   4   5   6   7   8   9   >