[PATCH] D113421: [clang][openmp][NFC] Remove arch-specific CGOpenMPRuntimeGPU files

JonChesterfield added a comment. I think __kmpc_get_warp_size needs an entry in Utils.cpp keepAlive(), and corresponding functions added to `deviceRTLs/*/src/target_impl.*` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113421/new/

[PATCH] D112680: [OpenMP] Lower printf to __llvm_omp_vprintf

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

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

JonChesterfield requested changes to this revision. JonChesterfield added a reviewer: ronl. JonChesterfield added a comment. This revision now requires changes to proceed. Marking as request changes because: `if (DEFINED ENV{ROCM_PATH})` Looking at environment variables is strictly worse than

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

JonChesterfield added a comment. I don't know about the MLIR parts, but replacing > find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS > /opt/rocm) with > find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATHS > ${ROCM_PATH}) seems an improvement,

[PATCH] D112680: [OpenMP] Lower printf to __llvm_omp_vprintf

JonChesterfield added a comment. Thanks for the review! Couple of helpers to split out and the rebase on main is messy, retesting now Comment at: clang/lib/CodeGen/CGGPUBuiltin.cpp:231 + VprintfFunc, {Args[0].getRValue(*this).getScalarVal(), BufferPtr, Size})); +}

[PATCH] D112504: [OpenMP] Wrap (v)printf in the new RT and use same handling for AMD

JonChesterfield added a comment. That's an interesting approach. Do you happen to know where I can find details of the data format behind that void*? Have been meaning to look at writing printf for amdgpu as host side decoding of that buffer. If the compiler knows how long it is, that would be

[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

[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

[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

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

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

[PATCH] D115153: clang/AMDGPU: Don't set implicit arg attribute to default size

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Great, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115153/new/ https://reviews.llvm.org/D115153 ___

[PATCH] D115283: [AMDGPU] Set "amdgpu_hostcall" module flag if an AMDGPU function has calls to device lib functions that use hostcalls.

JonChesterfield added a comment. In D115283#3182879 , @yaxunl wrote: > In D115283#3181128 , > @JonChesterfield wrote: > >> Not exactly that. The weak symbol isn't the function name, as that gets >> renamed or

[PATCH] D115283: [AMDGPU] Set "amdgpu_hostcall" module flag if an AMDGPU function has calls to device lib functions that use hostcalls.

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9425 +void AMDGPUTargetCodeGenInfo::checkFunctionCallABI(CodeGenModule , + SourceLocation CallLoc, Doesn't seem to check

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

JonChesterfield added a comment. In D109885#3198232 , @arsenm wrote: > Dropping the default install location from the default search hint is > entirely unreasonable That seems to be the feature request, though I haven't gone through the jira web to

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

JonChesterfield added inline comments. Comment at: clang/tools/amdgpu-arch/CMakeLists.txt:13 + +find_package(hsa-runtime64 QUIET 1.2.0 HINTS ${CMAKE_INSTALL_PREFIX} PATH ${ROCM_PATH}) if (NOT ${hsa-runtime64_FOUND}) arsenm wrote: > I also think

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

JonChesterfield added a comment. In D109885#3198296 , @arsenm wrote: > This isn't a feature, it's the introduction of a bug. It would regress people depending on the implicit pickup of /opt/rocm, leaving them with a straightforward workaround of

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

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

JonChesterfield added a comment. Diff was created without context unfortunately, scrolling through the cmake locally suggests variables have names of the form `LIBOMPTARGET_AMDGPU_FOO` though the namespacing isn't as consistent as it might be. Something like: if (not defined

[PATCH] D115661: [clang][amdgpu] - Choose when to promote VarDecl to address space 4.

JonChesterfield added a comment. Changing the has-constant-initialiser check to a more precise/robust one sounds reasonable to me. In D115661#3193157 , @arsenm wrote: > The backend also knows because the constant flag is set on the global > variable.

[PATCH] D115283: [AMDGPU] Set "amdgpu_hostcall" module flag if an AMDGPU function has calls to device lib functions that use hostcalls.

JonChesterfield added a comment. Not exactly that. The weak symbol isn't the function name, as that gets renamed or inlined. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115283/new/ https://reviews.llvm.org/D115283

[PATCH] D115283: [AMDGPU] Set "amdgpu_hostcall" module flag if an AMDGPU function has calls to device lib functions that use hostcalls.

JonChesterfield added a comment. Openmp defines a weak symbol in the hostcall_invoke function. Optimisation and deadstripping friendly, no compiler support necessary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115283/new/

[PATCH] D115661: [clang][amdgpu] - Choose when to promote VarDecl to address space 4.

JonChesterfield added a comment. Patch looks ok to me. This will fix the miscompile (we end up with a store to addrspace(4) at present) without upsetting whatever hacks rely on addrspace(4). @arsenm reasonable as a point fix? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D115661: [clang][amdgpu] - Choose when to promote VarDecl to address space 4.

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. That sounds good here. Infer addrspaces is pretty complicated, given marginal benefit for (4) this patch seems to hit the mark. Repository: rG LLVM Github Monorepo

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

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. D105221 so LGTM too Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/

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

JonChesterfield updated this revision to Diff 360447. JonChesterfield added a comment. - rebase on main Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904 Files: clang/lib/Driver/ToolChains/Clang.cpp

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG968899ad9cf1: [OpenMP][AMDGCN] Initial math headers support (authored by pdhaliwal, committed by JonChesterfield). Repository: rG LLVM Github

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

JonChesterfield added a comment. Landed on @pdhaliwal's behalf. My expectation is that this patch mostly works and the rough edges can be cleaned up once ocml is linked in and we can more easily run more applications through it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D105221: [openmp][nfc] Simplify macros guarding math complex headers

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

[PATCH] D105221: [openmp][nfc] Simplify macros guarding math complex headers

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3e649f8ef187: [openmp][nfc] Simplify macros guarding math complex headers (authored by JonChesterfield). Repository: rG LLVM Github Monorepo

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

JonChesterfield added a subscriber: ronlieb. JonChesterfield added a comment. @ronlieb bisected amdgpu crashing to this too, rocm 'veccopy' case tries to dereference 0. Might be the same failure mode as the above or a different one, the hsa error reporting is quite coarse grained. Suggest we

[PATCH] D106301: [OpenMP] Disable trap before unreachable for OpenMP device jobs

JonChesterfield added a comment. What's the problem with emitting llvm.trap in various unreachable places? Wondering if it also affects translating assert to an llvm.trap Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106301/new/

[PATCH] D106301: [OpenMP] Disable trap before unreachable for OpenMP device jobs

JonChesterfield added a comment. In D106301#2888170 , @jdoerfert wrote: > llvm.trap is preserved, thus branches to an llvm.trap are preserved. That's interesting. Consistent with IR in general, template int test(int x) { if (x < 42) {

[PATCH] D106301: [OpenMP] Disable trap before unreachable for OpenMP device jobs

JonChesterfield added a comment. I'm not sure about that - we could tie instcombine to -O0 or some similar proxy for debugging ve performance - but I'm practice it's fairly likely that most traps are compiler inserted so it probably works out the same. Conditional instcombine would let us

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

JonChesterfield added a comment. Thanks! Will take a look. Feel free to revert, I'll do so shortly if noone beats me to it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104904/new/ https://reviews.llvm.org/D104904

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

JonChesterfield added a comment. cstdlib test header contains // amdgcn already provides definition of fabs #ifndef __AMDGCN__ float fabs(float __x) { return __builtin_fabs(__x); } #endif If I delete or invert the ifndef >

[PATCH] D106542: [OPENMP]Fix PR49787: Codegen for calling __tgt_target_teams_nowait_mapper has too few arguments.

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

[PATCH] D106070: [HIP] Remove workaround in __clang_hip_runtime_wrapper.h

JonChesterfield added a comment. OK from the openmp gpu side as far as I can tell. This is probably another instance of where we really wanted _OPENMP_TARGET or similar. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106070/new/ https://reviews.llvm.org/D106070

[PATCH] D105937: [OpenMP] Encode `omp [...] assume[...]` assumptions with `omp[x]` prefix

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

[PATCH] D106870: [OpenMP] Multi architecture compilation support

JonChesterfield added a comment. There seems to be a bunch of different things in this patch. There's some driver plumbing to compile for more than one arch (presumably by calling the target compiler N times). That's a great feature, I want to build an application bthat can run on nvptx or

[PATCH] D106793: [OpenMP] Add a driver flag to enable the new device runtime library

JonChesterfield added a comment. Can we call this something other than new? We don't tend to remove command line arguments and this won't make much sense once it's the only runtime. I'd be inclined to add an argument called 'use_legacy_runtime' or similar, which defaults to true Repository:

[PATCH] D106793: [OpenMP] Add a driver flag to enable the new device runtime library

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

[PATCH] D105221: [openmp][nfc] Simplify macros guarding math complex headers

JonChesterfield added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/complex:21 #define __OPENMP_NVPTX__ #include <__clang_cuda_complex_builtins.h> #undef __OPENMP_NVPTX__ ^ this header does not look for a macro called __CUDA__ or include any

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added a comment. I don't like that this pulls in ockl automatically but don't think that's a blocker. OK on my side, @yaxunl? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105981/new/ https://reviews.llvm.org/D105981

[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:831-860 auto Kind = llvm::AMDGPU::parseArchAMDGCN(GpuArch); const StringRef CanonArch = llvm::AMDGPU::getArchNameAMDGCN(Kind); std::string LibDeviceFile =

[PATCH] D116906: [OpenMP][AMDGPU] Optimize the linked in math libraries

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Using the optimiser to hide errors in how we're pulling in libm is clearly not right, but it leaves us less obviously broken than we are at present. Repository: rG LLVM

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

JonChesterfield reopened this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I don't know the context of this patch but changing attribute((used)) to put things under compiler.used is definitely a breaking change. Please introduce a new attribute if

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

JonChesterfield added a comment. > Modulo optimizer bugs, __attribute__((used)) hasn't changed semantics. > If your downstream project does not handle llvm.compiler.used, maybe handle > it now :) There seems to be some confusion here. The 'downstream project' is openmp, which has worked around

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

JonChesterfield added a comment. In D97446#3241735 , @MaskRay wrote: > For your comment it appears an issue in the internalisation pass. It is > orthogonal to this patch. > Do you have a reduced example with reproduce instructions for the issue? I >

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

JonChesterfield added a comment. It looks deeply wrong to be marking global as either llvm.used or llvm.compiler.used based on the output file format. It should be based on the (purpose of) the global. In D97446#3241142 , @rnk wrote: > This change was

[PATCH] D117048: [OpenMP] Link the bitcode library late for device LTO

JonChesterfield added a comment. Do we have a hook where we could link it at, uh, link time on nvtptx without LTO? Amdgpu had a llvm-link already there. Always bothered me a little that we link a copy per TU then hope the optimiser sorts it out nicely. Repository: rG LLVM Github Monorepo

[PATCH] D117048: [OpenMP] Link the bitcode library late for device LTO

JonChesterfield added a comment. Oh, right. Nvptx is still lowering to machine code per-tu. We don't want the devicertl linking as machine code, so it has to go in per-tu. Or we could link nvptx as IR instead, and send that plus amdgpu down the same code path. Probably makes applications

[PATCH] D117048: [OpenMP] Link the bitcode library late for device LTO

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:268 + // Link the bitcode library late if we're using device LTO. + if (getDriver().isUsingLTO(/* IsOffload */ true)) +return; Can/should probably always link

[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

[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] D120272: [CUDA] Add driver support for compiling CUDA with the new driver

JonChesterfield added inline comments. Comment at: clang/lib/Driver/Driver.cpp:4132-4134 + Archs.insert(CudaArchToString(CudaArch::SM_35)); +else if (Kind == Action::OFK_HIP) + Archs.insert(CudaArchToString(CudaArch::GFX803)); tra wrote: > If we do

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

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

[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] D120353: [OpenMP] Add option to make offloading mandatory

JonChesterfield added a comment. Thanks! Seems a good thing to add to the offloading test runner, preferably in a separate change to avoid reverting this in case of unforeseen problems Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120353/new/

[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

[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

[PATCH] D122504: [OpenMP] Make Ctor / Dtor functions have external visibility

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Nice, thanks. Wonder if we want protected visibility as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122504/new/

[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

[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] D122069: [Object] Add binary format for bundling offloading metadata

JonChesterfield added reviewers: grokos, ABataev, ronlieb, tianshilei1992. JonChesterfield added a comment. Added some reviewers. I'd much prefer this used an existing binary format, DIY is prone to errors and maintenance hassles down the road. Don't care as much about which format as about it

[PATCH] D122352: [OpenMP] Do not create offloading entries for internal or hidden symbols

JonChesterfield added a comment. Sounds good to me too, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122352/new/ https://reviews.llvm.org/D122352 ___ cfe-commits mailing list

[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

[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] D118399: [OpenMP] Only generate runtime flags with host input

JonChesterfield added a comment. There's an existing flag for compile for device only, that's probably close enough to the right condition to not emit these flags. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118399/new/

[PATCH] D116542: [OpenMP] Add a flag for embedding a file into the module

JonChesterfield added a comment. This might be OK. If multiple translation units still get fed to llvm-link it'll be broken, which might be what we get on amdgpu at present. Not totally clear to me whether this should be compiler.used or .used, as it's used by something after clang but before

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield added a comment. Testing manually looks good, provided there's no command line argument involved. -rpath even composes sanely, so doing this plus passing -Wl,rpath= results in multiple embedded rpaths. At a loss as to why changing Options.td is working so poorly, though it may

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield created this revision. JonChesterfield added reviewers: jdoerfert, jhuber6, tianshilei1992, ye-luo. Herald added subscribers: guansong, yaxunl. JonChesterfield requested review of this revision. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. Openmp

[PATCH] D118495: [OpenMP] Accept shortened triples for -Xopenmp-target=

JonChesterfield added a comment. Do we have this same expansion logic in two places now? If so, probably want to factor it out to a function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118495/new/ https://reviews.llvm.org/D118495

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield added a comment. My plan is to feed executables to readelf -d. Commandline problems seem to be solvable by clean builds between changes, slow but feasible. Might upset the incremental buildbot Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404156. JonChesterfield added a comment. Herald added a subscriber: dang. - Now with commandline argument Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://reviews.llvm.org/D118493

[PATCH] D116544: [Clang] Introduce Clang Linker Wrapper Tool

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. I think this is reasonable. It's a lot of boilerplate to put the interception shim in the right place, but we do want a specific point to inject offloading functionality and

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. In D118493#3284781 , @thakis wrote: > In D118493#3284663 , > @JonChesterfield wrote: > >> In D118493#3284617 , @thakis wrote: >> >>>

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. Yep. I'm not totally confident what windows builds will embed in the dynamic table - might be windows style \ or it might be linux style /. Or it might be \\, which can be matched as in D109057 . I think I'm going with the very

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404536. JonChesterfield added a comment. - More paranoid regex - not sure what windows uses as the path separator in the dynamic table Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a reviewer: kparzysz. JonChesterfield added a subscriber: kparzysz. JonChesterfield added a comment. This test doesn't work on hexagon because hexagon doesn't have a linker (??), emailed @kparzysz asking how to disable tests on hexagon as I can't find a likely looking

[PATCH] D118493: Set rpath on openmp executables

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa80d5c34e4b9: Set rpath on openmp executables (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added inline comments. Comment at: clang/test/OpenMP/implicit_rpath.c:28 +// CHECK-COMPOSABLE: ({{R|RUN}}PATH) Library {{r|run}}path: [early:late:{{.*}}llvm/lib] + +int main() {} This ^ probably has path separator issues on windows, will try to

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. Apologies for the noise, turns out 'llvm/lib' and 'llvm/lib64' are not the only two options. Made it more permissive and reapplied. Also looks like ppc64le defaults to rpath as opposed to runpath, so that's worth knowing. Repository: rG LLVM Github Monorepo

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield added a comment. In D118493#3284617 , @thakis wrote: > looks like the mac linker doesn't like this test either: > http://45.33.8.238/macm1/26834/step_7.txt Error message is ld: file too small (length=8) file

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield added a comment. Slightly stalled on testing - I'd like to emit the object and feed it to readelf, something like: `// RUN: %clang %s -o %t && llvm-readelf --dynamic-table %t | FileCheck %s --check-prefixes=CHECK` which errors with cannot find -lomp. I feel there should be a

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404193. JonChesterfield added a comment. - set rpath after libomptarget, reads better than between the two -l flags Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404496. JonChesterfield added a comment. - Test rpath and runpath setting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://reviews.llvm.org/D118493 Files:

[PATCH] D118493: [WIP]Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404497. JonChesterfield added a comment. - composes with user specified rpath flags Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118493/new/ https://reviews.llvm.org/D118493 Files:

[PATCH] D118493: Set rpath on openmp executables

JonChesterfield updated this revision to Diff 404498. JonChesterfield added a comment. Herald added a project: OpenMP. Herald added a subscriber: openmp-commits. - Disable implicit rpath for the tests so we can be certain the tests aren't picking up unrelated libs Repository: rG LLVM Github

[PATCH] D118493: Set rpath on openmp executables

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

<    2   3   4   5   6   7   8   9   >