[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

fjricci added inline comments. Comment at: lib/Driver/ToolChain.cpp:851 + XOpenMPTargetArg->setBaseArg(A); + A = XOpenMPTargetArg.release(); + DAL->append(A); Hahnfeld wrote: > This is a memory leak that is currently triggered in >

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

Hahnfeld added inline comments. Comment at: lib/Driver/ToolChain.cpp:851 + XOpenMPTargetArg->setBaseArg(A); + A = XOpenMPTargetArg.release(); + DAL->append(A); This is a memory leak that is currently triggered in

Re: [PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

I am aware of the failure, I am attempting to push a fix as we speak!Thanks for the patience.--DoruFrom:        Aleksey Shlyapnikov via Phabricator To:        gheorghe-teod.ber...@ibm.com, hfin...@anl.gov, hahnf...@itc.rwth-aachen.de, cber...@us.ibm.com,

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

alekseyshl added a comment. http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/7010 is unhappy about this change, please fix. https://reviews.llvm.org/D34784 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 110007. gtbercea added a comment. Fix test comments. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td include/clang/Driver/ToolChain.h lib/Driver/Compilation.cpp

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel accepted this revision. hfinkel added a comment. This revision is now accepted and ready to land. LGTM. Thanks for all of your work on this! Comment at: test/Driver/openmp-offload.c:603 + +/// Check -Xopenmp-target=powerpc64le-ibm-linux-gnu -march=pwr8 is passed when

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 109939. gtbercea added a comment. Fix -march special casing. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td include/clang/Driver/ToolChain.h lib/Driver/Compilation.cpp

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added inline comments. Comment at: lib/Driver/ToolChain.cpp:808 + continue; + } else if (XOpenMPTargetNoTriple) +// Passing device args: -Xopenmp-target -opt=val. hfinkel wrote: > Please include {} around this else-if code, even

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 109938. gtbercea added a comment. Address comments. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td include/clang/Driver/ToolChain.h lib/Driver/Compilation.cpp

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added inline comments. Comment at: lib/Driver/ToolChain.cpp:808 + continue; + } else if (XOpenMPTargetNoTriple) +// Passing device args: -Xopenmp-target -opt=val. Please include {} around this else-if code, even though it is not

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 109904. gtbercea added a comment. Don't exclude flags when host matches offload toolchain. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td include/clang/Driver/ToolChain.h

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 109903. gtbercea added a comment. New way to handle OpenMP target flags. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td include/clang/Driver/ToolChain.h

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added a comment. This is much closer to what I had in mind, thanks! Now I think we're in a position to make this work for more than just the CUDA target. It looks like the added code is now: 1. Remove -march from the translated arguments (because any existing -march would apply only

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added a comment. @hfinkel I think I have something that works which is similar to what you were requesting. Please let me know your thoughts! Thanks, --Doru https://reviews.llvm.org/D34784 ___ cfe-commits mailing list

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 105932. gtbercea added a comment. Address comments. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td lib/Driver/ToolChains/Cuda.cpp test/Driver/openmp-offload.c Index:

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:474 +for (StringRef Opt : OptList) { + AddMArchOption(DAL, Opts, Opt); +} gtbercea wrote: > hfinkel wrote: > > gtbercea wrote: > > > hfinkel wrote: > > > > Shouldn't you be

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:474 +for (StringRef Opt : OptList) { + AddMArchOption(DAL, Opts, Opt); +} hfinkel wrote: > gtbercea wrote: > > hfinkel wrote: > > > Shouldn't you be adding all of the

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:474 +for (StringRef Opt : OptList) { + AddMArchOption(DAL, Opts, Opt); +} gtbercea wrote: > hfinkel wrote: > > Shouldn't you be adding all of the options, not just the -march=

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:478 +auto MArchList = DAL->getAllArgValues(options::OPT_march_EQ); +assert(MArchList.size() < 2 && "At most one GPU arch allowed."); +if (MArchList.empty()) hfinkel wrote: >

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 105355. gtbercea added a comment. Address Comments. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td lib/Driver/ToolChains/Cuda.cpp test/Driver/openmp-offload.c Index:

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 105354. gtbercea added a comment. Address comments. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td lib/Driver/Driver.cpp lib/Driver/ToolChains/Clang.cpp

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added inline comments. Comment at: test/Driver/openmp-offload.c:607 + +// CHK-FOPENMP-EQ-TARGET: clang{{.*}} argument unused during compilation: '-Xopenmp-target=powerpc64le-ibm-linux-gnu -march=pwr8' + hfinkel wrote: > I don't see why you'd check that

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 105280. gtbercea marked 3 inline comments as done. gtbercea added a comment. Address comments. https://reviews.llvm.org/D34784 Files: include/clang/Basic/DiagnosticDriverKinds.td include/clang/Driver/Options.td lib/Driver/ToolChains/Cuda.cpp

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added inline comments. Comment at: lib/Driver/ToolChains/Cuda.cpp:443 + +// Get the compute capability from the -fopenmp-targets flag. +// The default compute capability is sm_20 since this is a CUDA hfinkel wrote: > Is this first sentence

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added inline comments. Comment at: include/clang/Driver/Options.td:453 +def Xopenmp_target : Separate<["-"], "Xopenmp-target">, + HelpText<"Pass arguments to target offloading toolchain.">; +def Xopenmp_target_EQ : JoinedAndSeparate<["-"], "Xopenmp-target=">,

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added a comment. @hfinkel I've add the flag as suggested. There is one minor change, I used "=" instead of ":" when specifying the toolchain/triple. I also support the triple being omitted when there is only one offloading toolchain specified with -fopenmp-targets.

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 104962. gtbercea added a comment. Check -fopenmp-targets has one entry when using default toolchain in -Xopenmp-target. https://reviews.llvm.org/D34784 Files: include/clang/Driver/Options.td lib/Driver/ToolChains/Cuda.cpp

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 104960. gtbercea retitled this revision from "[OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading " to "[OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading". gtbercea added a

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added a comment. ... > Okay, good, this is exactly where I was going when I said I was worried about generalization. -march seems like one of many flags I might want to pass to the target compilation. Moreover, it doesn't seem special in what regard.

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added a comment. In https://reviews.llvm.org/D34784#795988, @hfinkel wrote: > In https://reviews.llvm.org/D34784#795980, @gtbercea wrote: > > > In https://reviews.llvm.org/D34784#795934, @hfinkel wrote: > > > > > In https://reviews.llvm.org/D34784#795871, @gtbercea wrote: > > > > > > >

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added a comment. In https://reviews.llvm.org/D34784#795980, @gtbercea wrote: > In https://reviews.llvm.org/D34784#795934, @hfinkel wrote: > > > In https://reviews.llvm.org/D34784#795871, @gtbercea wrote: > > > > > In https://reviews.llvm.org/D34784#795367, @hfinkel wrote: > > > > > > >

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added a comment. In https://reviews.llvm.org/D34784#795934, @hfinkel wrote: > In https://reviews.llvm.org/D34784#795871, @gtbercea wrote: > > > In https://reviews.llvm.org/D34784#795367, @hfinkel wrote: > > > > > In https://reviews.llvm.org/D34784#795353, @gtbercea wrote: > > > > > > >

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added a comment. In https://reviews.llvm.org/D34784#795871, @gtbercea wrote: > In https://reviews.llvm.org/D34784#795367, @hfinkel wrote: > > > In https://reviews.llvm.org/D34784#795353, @gtbercea wrote: > > > > > In https://reviews.llvm.org/D34784#795287, @hfinkel wrote: > > > > > > >

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added a comment. In https://reviews.llvm.org/D34784#795367, @hfinkel wrote: > In https://reviews.llvm.org/D34784#795353, @gtbercea wrote: > > > In https://reviews.llvm.org/D34784#795287, @hfinkel wrote: > > > > > What happens if you have multiple targets? Maybe this should be > > >

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added a comment. In https://reviews.llvm.org/D34784#795353, @gtbercea wrote: > In https://reviews.llvm.org/D34784#795287, @hfinkel wrote: > > > What happens if you have multiple targets? Maybe this should be > > -fopenmp-targets-arch=foo,bar,whatever? > > > > Once this all lands, please

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea added a comment. In https://reviews.llvm.org/D34784#795287, @hfinkel wrote: > What happens if you have multiple targets? Maybe this should be > -fopenmp-targets-arch=foo,bar,whatever? > > Once this all lands, please make sure that you add additional test cases > here. Make sure that

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

hfinkel added a comment. What happens if you have multiple targets? Maybe this should be -fopenmp-targets-arch=foo,bar,whatever? Once this all lands, please make sure that you add additional test cases here. Make sure that the arch is passed through to the ptx and cuda tools as it should be.

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea updated this revision to Diff 104532. Repository: rL LLVM https://reviews.llvm.org/D34784 Files: include/clang/Driver/Options.td lib/Driver/ToolChains/Cuda.cpp test/Driver/openmp-offload.c Index: test/Driver/openmp-offload.c

[PATCH] D34784: [OpenMP] Add flag for specifying the target device architecture for OpenMP device offloading

gtbercea created this revision. OpenMP has the ability to offload target regions to devices which may have different architectures. A new -fopenmp-target-arch flag is introduced to specify the device architecture. In this patch I use the new flag to specify the compute capability of the