yaxunl marked 19 inline comments as done.
yaxunl added a comment.
In https://reviews.llvm.org/D45212#1105177, @tra wrote:
> Hi,
>
> Sorry about the long silence. I'm back to continue the reviews. I'll handle
> what I can today and will continue with the rest on Tuesday.
>
> It looks like patch
tra added a comment.
One more thing -- it would be really good to add some tests to make sure your
commands are constructed the way you want.
https://reviews.llvm.org/D45212
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
tra added a comment.
Hi,
Sorry about the long silence. I'm back to continue the reviews. I'll handle
what I can today and will continue with the rest on Tuesday.
It looks like patch description needs to be updated:
> Use clang-offload-bindler to create binary for device ISA.
I don't see
yaxunl added a comment.
Hi Artem,
I've addressed your comments. Any further changes are needed? Thanks.
https://reviews.llvm.org/D45212
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
yaxunl added a comment.
ping
https://reviews.llvm.org/D45212
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yaxunl updated this revision to Diff 145297.
yaxunl added a comment.
clean up code and separate action builder to another review.
https://reviews.llvm.org/D45212
Files:
include/clang/Driver/Options.td
lib/Driver/ToolChains/Cuda.cpp
lib/Driver/ToolChains/Cuda.h
Index:
yaxunl marked 14 inline comments as done.
yaxunl added a comment.
ping
https://reviews.llvm.org/D45212
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
yaxunl updated this revision to Diff 143320.
yaxunl marked an inline comment as done.
yaxunl added a comment.
Remove manually added passes from opt and add -mtriple.
https://reviews.llvm.org/D45212
Files:
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Action.h
yaxunl marked an inline comment as done.
yaxunl added inline comments.
Comment at: lib/Driver/ToolChains/Cuda.cpp:498-501
+OptArgs.push_back(mcpustr);
+OptArgs.push_back("-dce");
+OptArgs.push_back("-sroa");
+OptArgs.push_back("-globaldce");
yaxunl updated this revision to Diff 143307.
yaxunl added a comment.
minor bug fix: do not add CUDA specific link options for HIP.
https://reviews.llvm.org/D45212
Files:
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Action.h
include/clang/Driver/Options.td
yaxunl updated this revision to Diff 143298.
yaxunl edited the summary of this revision.
yaxunl added a comment.
sync to ToT.
https://reviews.llvm.org/D45212
Files:
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Action.h
include/clang/Driver/Options.td
yaxunl updated this revision to Diff 143218.
yaxunl marked 14 inline comments as done.
yaxunl added a comment.
Revised by Artem's comments.
https://reviews.llvm.org/D45212
Files:
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Action.h
include/clang/Driver/Options.td
yaxunl marked 21 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Driver/Options.td:552-553
+def : Joined<["--"], "offload-arch=">, Alias;
+def offload_archs : Joined<["--"], "offload-archs=">, Flags<[DriverOption]>,
+ HelpText<"List of offload
tra requested changes to this revision.
tra added a subscriber: jlebar.
tra added a comment.
This revision now requires changes to proceed.
Sorry about the delay. I've been out for few days.
Comment at: include/clang/Driver/Options.td:552-553
+def : Joined<["--"],
Hahnfeld added a comment.
In https://reviews.llvm.org/D45212#1066842, @rjmccall wrote:
> I'd still prefer if someone with more driver-design expertise weighed in, but
> we might not have any specialists there.
I think you should at least give @tra the possibility to take a look. Last time
we
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.
I'd still prefer if someone with more driver-design expertise weighed in, but
we might not have any specialists there.
LGTM, although you might consider changing your tests a bit:
Hahnfeld added a comment.
In https://reviews.llvm.org/D45212#1063186, @yaxunl wrote:
> In https://reviews.llvm.org/D45212#1060628, @Hahnfeld wrote:
>
> > Can this revision be split further? The summary mentions many things that
> > might make up multiple independent changes...
>
>
> I separated
yaxunl added a comment.
In https://reviews.llvm.org/D45212#1060628, @Hahnfeld wrote:
> Can this revision be split further? The summary mentions many things that
> might make up multiple independent changes...
I separated file type changes to https://reviews.llvm.org/D45489 and deferred
some
yaxunl updated this revision to Diff 141863.
yaxunl marked 2 inline comments as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Separate file type changes to another patch.
https://reviews.llvm.org/D45212
Files:
include/clang/Driver/Options.td
yaxunl marked 2 inline comments as done.
yaxunl added inline comments.
Comment at: lib/Driver/Driver.cpp:2267
+if ((IA->getType() != types::TY_CUDA) &&
+IA->getType() != types::TY_HIP) {
// The builder will ignore this input.
Hahnfeld added a comment.
Can this revision be split further? The summary mentions many things that might
make up multiple independent changes...
Comment at: lib/Driver/ToolChains/Cuda.cpp:263
+// HIP needs c++11.
+CC1Args.push_back("-std=c++11");
+// Skip CUDA
rjmccall added a comment.
This looks okay to me, but I think it would better if someone with more
expertise in the design of the driver and frontend code could review this.
Comment at: lib/Driver/Driver.cpp:2267
+if ((IA->getType() != types::TY_CUDA) &&
+
yaxunl updated this revision to Diff 141221.
yaxunl marked an inline comment as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Revised by reviewers' comments, including comments from previous review.
https://reviews.llvm.org/D45212
Files:
yaxunl marked 4 inline comments as done.
yaxunl added inline comments.
Comment at: include/clang/Basic/Cuda.h:61
+ GFX900,
+ GFX902,
LAST,
Hahnfeld wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > Does this actually have anything to
Hahnfeld added inline comments.
Comment at: include/clang/Basic/Cuda.h:61
+ GFX900,
+ GFX902,
LAST,
rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > Does this actually have anything to do with HIP? You have a lot of
> > > changes in this patch
rjmccall added inline comments.
Comment at: include/clang/Basic/Cuda.h:61
+ GFX900,
+ GFX902,
LAST,
yaxunl wrote:
> rjmccall wrote:
> > Does this actually have anything to do with HIP? You have a lot of changes
> > in this patch which seem to just be
26 matches
Mail list logo
