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
> `tests/Driver/o
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
`tests/Driver/openmp-offload-gpu.c
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, caom...@us.ibm.com,
a.bat...@hotmail.comCc:
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
http://lists.llv
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
lib/Driver/ToolCh
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
c
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
lib/Drive
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 th
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
lib/Driver/ToolCha
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 nec
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
lib/Dr
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
lib/Driver/Compilation.c
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 fo
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
cfe-commits@lists.l
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: test/Driver/
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 add
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 options,
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=
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:
> Ca
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: test/Driver/
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
lib/Driver/ToolChains/Co
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
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
test/D
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 accura
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=">,
---
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.
https://reviews.ll
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
test/Driver/openmp-offload.
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 c
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.
>>
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:
> > >
> > > > I
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:
> > >
> > > > In
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:
> > >
> > > > I
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:
> > >
> > > > Wh
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
> > > -fop
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
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 th
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. M
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
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
under
39 matches
Mail list logo
