[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. I have just pushed a fix, revision 310433. https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

alekseyshl added a comment. Driver/openmp-offload.c still fails on http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/7038, please fix. https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. In https://reviews.llvm.org/D29654#835548, @hfinkel wrote: > In https://reviews.llvm.org/D29654#835392, @gtbercea wrote: > > > In https://reviews.llvm.org/D29654#835371, @arphaman wrote: > > > > > The last RUN line in the new commit triggers the same assertion failure:

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

hfinkel added a comment. In https://reviews.llvm.org/D29654#835392, @gtbercea wrote: > In https://reviews.llvm.org/D29654#835371, @arphaman wrote: > > > The last RUN line in the new commit triggers the same assertion failure: > ... > Hi Alex, I'm not sure why it's failing as I can't reproduce

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. No, entry 1 has a different pointer (0x000111c01320 vs 0x000111c017d0). Why is there a `DependentBoundArch` if it's always empty? https://reviews.llvm.org/D29654 ___ cfe-commits mailing list

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. In https://reviews.llvm.org/D29654#835507, @arphaman wrote: > In https://reviews.llvm.org/D29654#835501, @gtbercea wrote: > > > Is that the last access to CachedResults before the error? > > > Is the assertion the last access? Yes. > > There must be a discrepancy

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. In https://reviews.llvm.org/D29654#835501, @gtbercea wrote: > Is that the last access to CachedResults before the error? Is the assertion the last access? Yes. There must be a discrepancy between `UI.DependentBoundArch` in the loop above and `BoundArch` that's used

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. Is that the last access to CachedResults before the error? https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. In https://reviews.llvm.org/D29654#835429, @arphaman wrote: > The "x86_64-apple-darwin17.0.0-x86_64-host" triple looks suspicious though It looks like the triple is in the list though: second = "x86_64-apple-darwin17.0.0-x86_64-host it is entry [1].

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The "x86_64-apple-darwin17.0.0-x86_64-host" triple looks suspicious though https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. If you look at the map then you can see that it contains very similar keys, but not the exact one: first = { first = 0x000111c017d0 second = "x86_64-apple-darwin17.0.0-host" } and first = { first =

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The cached results map doesn't have the key: (lldb) p CachedResults (std::__1::map >, clang::driver::InputInfo, std::__1::less > >, std::__1::allocator

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. In https://reviews.llvm.org/D29654#835392, @gtbercea wrote: > In https://reviews.llvm.org/D29654#835371, @arphaman wrote: > > > The last RUN line in the new commit triggers the same assertion failure: > > > > Assertion failed: (CachedResults.find(ActionTC) !=

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. In https://reviews.llvm.org/D29654#835371, @arphaman wrote: > The last RUN line in the new commit triggers the same assertion failure: > > Assertion failed: (CachedResults.find(ActionTC) != CachedResults.end() && > "Result does not exist??"), function

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The last RUN line in the new commit triggers the same assertion failure: Assertion failed: (CachedResults.find(ActionTC) != CachedResults.end() && "Result does not exist??"), function BuildJobsForActionNoCache, file

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. Looks like it's still failing unfortunately: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental_check/39182/console https://reviews.llvm.org/D29654 ___ cfe-commits mailing list

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. In https://reviews.llvm.org/D29654#835256, @arphaman wrote: > Great, thanks! I think that you can just revert my revert with the fix > applied in one commit Hi Alex, I just commited the changes again. Let me know if it still fails for you. I think the issue was

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. Great, thanks! I think that you can just revert my revert with the fix applied in one commit https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. In https://reviews.llvm.org/D29654#835045, @arphaman wrote: > Hi @gtbercea, > I couldn't reply to the email as cfe-commits didn't even register this > commit somehow, so I'm replying here. > > Unfortunately I had to revert this commit (r310291), + two others for a

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. The revert commit is r310345 btw https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

arphaman added a comment. Hi @gtbercea, I couldn't reply to the email as cfe-commits didn't even register this commit somehow, so I'm replying here. Unfortunately I had to revert this commit (r310291), + two others for a clean revert (r310300 and r310332) because it caused a test failure on

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 110056. gtbercea added a comment. Add -no-canonical-prefixes to tests. https://reviews.llvm.org/D29654 Files: lib/Driver/Driver.cpp lib/Driver/ToolChains/Clang.cpp lib/Driver/ToolChains/CommonArgs.cpp lib/Driver/ToolChains/CommonArgs.h

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 104535. gtbercea added a comment. [Update regression tests] Add a test for propagating the compute capability to the OpenMP device offloading toolchain which targets NVIDIA GPUs. This is a test for patch D34784 which is

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM Repository: rL LLVM https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 98766. gtbercea added a comment. Address comments. Repository: rL LLVM https://reviews.llvm.org/D29654 Files: lib/Driver/Driver.cpp lib/Driver/ToolChains/Clang.cpp lib/Driver/ToolChains/CommonArgs.cpp lib/Driver/ToolChains/CommonArgs.h

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:388 + + // add paths specified in LIBRARY_PATH environment variable as -L options. + addDirectoryList(Args, CmdArgs, "-L", "LIBRARY_PATH"); Comment sentences should start with a capital

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. ping Repository: rL LLVM https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea added a comment. @rnk any further thought on the changes to this patch? Thanks :) Repository: rL LLVM https://reviews.llvm.org/D29654 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 97331. gtbercea added a comment. Change output of unbundler to produce cubin files when using OpenMP to offload to NVIDIA GPUs. Repository: rL LLVM https://reviews.llvm.org/D29654 Files: lib/Driver/Driver.cpp lib/Driver/ToolChains/Clang.cpp

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 95737. gtbercea marked an inline comment as done. gtbercea added a comment. Avoid renaming by enabling PTXAS to generate an output file with the appropriate extension, in this case a cubin extension. Repository: rL LLVM https://reviews.llvm.org/D29654

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea marked 4 inline comments as done. gtbercea added inline comments. Comment at: test/Driver/openmp-offload.c:594 +/// Check cubin file generation and usage by nvlink +// RUN: %clang -### -fopenmp=libomp -fopenmp-targets=nvptx64-nvidia-cuda -save-temps

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 95175. gtbercea added a comment. Use the rename() utility function of LLVM for renaming the PTXAS output before invoking NVLINK. Repository: rL LLVM https://reviews.llvm.org/D29654 Files: lib/Driver/Driver.cpp lib/Driver/ToolChains/CommonArgs.cpp

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 94475. gtbercea added a comment. Address some of the reviews. Repository: rL LLVM https://reviews.llvm.org/D29654 Files: lib/Driver/Driver.cpp lib/Driver/ToolChains/CommonArgs.cpp lib/Driver/ToolChains/CommonArgs.h

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

rnk added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:324 + "CUDA toolchain not expected for an OpenMP host device."); + if (JA.isDeviceOffloading(Action::OFK_OpenMP)) { +if (Output.isFilename()) { This entire if block constructs a

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

Hahnfeld added a reviewer: rnk. Hahnfeld added a subscriber: rnk. Hahnfeld added a comment. Please format all comments as full sentences. Comment at: lib/Driver/ToolChains/Cuda.cpp:338 +Args.getAllArgValues(options::OPT_march_EQ); +assert(GPUArchs.size() == 1 &&

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

jlebar added a comment. With your latest set of updates, I am not sure which, if any, patches you need me to take another look at. Unfortunately I don't have a ton of time for CUDA stuff these days, so where possible I'd prefer to shunt reviews over to hfinkel or chandlerc. But do let me

[PATCH] D29654: [OpenMP] Integrate OpenMP target region cubin into host binary

gtbercea updated this revision to Diff 93192. gtbercea added a comment. Herald added a subscriber: rengolin. Update patch to reflect latest source code changes. Repository: rL LLVM https://reviews.llvm.org/D29654 Files: lib/Driver/ToolChains/CommonArgs.cpp