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

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

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

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

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

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

[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 '/Users/thakis/src/llvm-proj

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

[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

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 http

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

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

[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] D116542: [OpenMP] Add a flag for embedding a file into the module

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Thanks for sticking with me on this! LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116542/new/ https://reviews.llvm.org/D11

[PATCH] D116543: [OpenMP] Embed device files into the host IR

JonChesterfield added a comment. Description and test have slightly diverged from implementation - filename is appended to disambiguate, but the filecheck regex only looks at the prefix and the name described in the commit message is missing the filename Repository: rG LLVM Github Monorepo

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1760 + for (StringRef OffloadObject : CGOpts.OffloadObjects) { +auto FilenameAndSection = OffloadObject.split(','); +llvm::ErrorOr> ObjectOrErr = JonChesterfield wrote:

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1771 +SmallString<128> SectionName( +{".llvm.offloading.", std::get<1>(FilenameAndSection)}); +llvm::EmbedBufferInModule(*M, **ObjectOrErr, SectionName); OK, so o

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1760 + for (StringRef OffloadObject : CGOpts.OffloadObjects) { +auto FilenameAndSection = OffloadObject.split(','); +llvm::ErrorOr> ObjectOrErr = Could we have a type he

[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

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

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

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

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

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

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

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

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

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

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

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

JonChesterfield added inline comments. Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4934 assert(Old->hasOneUse() && "llvm.embedded.module can only be used once in llvm.compiler.used"); GV->takeName(Old); here's an assert that this f

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

JonChesterfield added inline comments. Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4891 GlobalVariable *Used = collectUsedGlobalVariables(M, UsedGlobals, true); for (auto *GV : UsedGlobals) { if (GV->getName() != "llvm.embedded.module" && Thi

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

JonChesterfield requested changes to this revision. JonChesterfield added a comment. This revision now requires changes to proceed. I don't think this works for multiple files. The test only tries one and misses all the parsing handling. Comment at: clang/include/clang/Basic/C

[PATCH] D116541: [OpenMP] Introduce new flag to change offloading driver pipeline

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. This looks reasonable to me, though I'd prefer we keep the forward declare. The scalar->vector transform is mechanical and the `if (Args.hasArg(options::OPT_fopenmp_new_drive

[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. The asm variable used by rocm aomp is zero overhead, needs no compiler support and works exactly as one would wish under inlining or code elimination. The main argument against that approach seems to be it's an abi break, much like this patch was, and that it is

[PATCH] D117806: [OpenMP] Change default visibility to protected for device declarations

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

[PATCH] D117806: [OpenMP] Change default visibility to protected for device declarations

JonChesterfield added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5830 + // host, makes the system more robust, and improves performance. + if (IsOpenMPDevice) { +CmdArgs.push_back("-fvisibility"); I think we need some more code here

[PATCH] D117806: [OpenMP] Change default visibility to protected for device declarations

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

[PATCH] D70549: [OPENMP]Fix PR41826: symbols visibility in device code.

JonChesterfield added subscribers: jhuber6, JonChesterfield. JonChesterfield added a comment. Herald added subscribers: asavonic, sstefan1, yaxunl. @ABataev @jhuber6 and I would like to change this to annotate variables with whatever visibility the user asked for. Can you recall if the concern a

[PATCH] D117634: [OpenMP] Expand short verisions of OpenMP offloading triples

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. LG, thanks! Comment at: clang/lib/Driver/Driver.cpp:778 + // We want to normalize the shortened versions of triples passed in to + // the

[PATCH] D117706: [openmp] Unconditionally set march commandline argument

JonChesterfield added a comment. Landed with you as author and me as reviewer as that seems a more accurate statement than the default. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117706/new/ https://reviews.llvm.org/D117706 ___

[PATCH] D117706: [openmp] Unconditionally set march commandline argument

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa9935b5db706: [openmp] Unconditionally set march commandline argument (authored by jhuber6, committed by JonChesterfield). Repository: rG LLVM Git

[PATCH] D117706: [openmp] Unconditionally set march commandline argument

JonChesterfield created this revision. JonChesterfield added a reviewer: jhuber6. Herald added subscribers: kerbowa, guansong, tpr, yaxunl, nhaehnle, jvesely. JonChesterfield requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1. Hera

[PATCH] D117246: [OpenMP] Add support for linking AMDGPU images

JonChesterfield added a comment. A bit nervous about the automatic appending of host library search paths for the device link but I see that's pre-existing in nvptx. I'll check whether -### changes behaviour as expected and approve if it does Comment at: clang/lib/Driver/Tool

[PATCH] D117362: [OpenMP] Remove hidden visibility for declare target variables

JonChesterfield added inline comments. Comment at: clang/lib/AST/Decl.cpp:1080 + if (const VarDecl *VD = dyn_cast(D)) +if (OMPDeclareTargetDeclAttr::isDeclareTargetDeclaration(VD)) + return LinkageInfo(LV.getLinkage(), DefaultVisibility, false); Possibl

[PATCH] D117362: [OpenMP] Remove hidden visibility for declare target variables

JonChesterfield added a comment. If I'm following correctly, this broke the amdgpu buildbot and it has been moved into staging as a workaround. I haven't debugged what breaks but 'DefaultVisibility' is relatively likely to behave differently on cuda vs hsa systems so that's my first guess. =

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

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

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

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

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

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

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

[PATCH] D96248: [OpenMP][AMDGPU] Add support for linking libomptarget bitcode

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

[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] 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. In D109885#3203412 , @arsenm wrote: > We should have versioned libraries and well defined ABI breaks. The build > should know what runtime versions it requires and account when searching If we're playing that game, ROCm

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

JonChesterfield added a comment. In D109885#3198375 , @arsenm wrote: > In D109885#3198340 , > @JonChesterfield wrote: > >> I'm very mistrusting of mixing a rocm toolchain with a trunk toolchain as >> they're bot

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

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

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

[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 (LIBOMPTARGET_AM

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

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

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

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

[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 ___ cfe-commit

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

[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 &CGM, + SourceLocation CallLoc, Doesn't seem to check an

[PATCH] D115157: [openmp] Default to new rtl for amdgpu

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6bb2a4f3e654: [openmp] Default to new rtl for amdgpu (authored by JonChesterfield). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D115157: [openmp] Default to new rtl for amdgpu

JonChesterfield added a comment. It's a bit hard to tell. Definitely might work. I'm hoping someone will feed this into the internal CI infra. @ronlieb any objection to making this change on trunk and reverting it internally if things break there? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D115157: [openmp] Default to new rtl for amdgpu

JonChesterfield created this revision. JonChesterfield added reviewers: ronlieb, jhuber6, jdoerfert, pdhaliwal. Herald added subscribers: dang, kerbowa, guansong, t-tye, tpr, dstuttard, yaxunl, nhaehnle, jvesely, kzhuravl. JonChesterfield requested review of this revision. Herald added subscribers

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

<    1   2   3   4   5   6   7   8   9   >