tra accepted this revision.
tra added a comment.
This revision is now accepted and ready to land.
LGTM.
Comment at: test/Driver/cuda_phases.cu:48
@@ +47,3 @@
+//
+// Test two gpu architecture with complete compilation.
+//
architecture*s*.
There are few more cop
sfantao marked an inline comment as done.
Comment at: test/Driver/cuda_phases.cu:2
@@ +1,3 @@
+// Tests the phases generated for a CUDA offloading target for different
+// combinations of:
+// - Number of gpu architectures;
Oh, ok. Thought that the registration of
sfantao updated this revision to Diff 63889.
sfantao added a comment.
- Add comments and REQUIRE directives to test.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
lib/Driver/Action.cpp
lib/Driver/Dr
tra added inline comments.
Comment at: test/Driver/cuda_phases.cu:1
@@ +1,2 @@
+// RUN: %clang -target powerpc64le-ibm-linux-gnu -ccc-print-phases
--cuda-gpu-arch=sm_30 %s 2>&1 \
+// RUN: | FileCheck -check-prefix=BIN %s
Few words describing the test would be nic
sfantao updated this revision to Diff 63880.
sfantao added a comment.
- Add test to check the generated phases for CUDA.
- Fix typo in comment.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
lib/Driver
tra added a comment.
The changes look good.
Now we just need some tests. Something along the lines of test/Driver/phases.c
should do.
http://reviews.llvm.org/D18171
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi
sfantao added inline comments.
Comment at: lib/Driver/Action.cpp:191-202
@@ +190,14 @@
+ auto I = getInputs().begin();
+ auto E = getInputs().end();
+ if (I == E)
+return;
+
+ // Skip host action
+ if (HostTC)
+++I;
+
+ auto TI = DevToolChains.begin();
+ for (; I !=
sfantao updated this revision to Diff 63865.
sfantao marked an inline comment as done.
sfantao added a comment.
- Modify assertion to check that sizes of input dependences and device
toolchains is consistent.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang
tra added inline comments.
Comment at: lib/Driver/Action.cpp:191-202
@@ +190,14 @@
+const OffloadActionWorkTy &Work) const {
+ auto I = getInputs().begin();
+ auto E = getInputs().end();
+ if (I == E)
+return;
+
+ // Skip host action
+ if (HostTC)
+++I;
+
+ auto
sfantao added a comment.
Hi Art,
Thanks for the review! Addressed your comments in the last diff.
Thanks again,
Samuel
Comment at: lib/Driver/Action.cpp:191-202
@@ +190,14 @@
+const OffloadActionWorkTy &Work) const {
+ auto I = getInputs().begin();
+ auto E = getInputs()
sfantao updated this revision to Diff 63852.
sfantao added a comment.
- Add one more assert.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
lib/Driver/Action.cpp
lib/Driver/Driver.cpp
lib/Driver/To
sfantao updated this revision to Diff 63850.
sfantao marked 4 inline comments as done.
sfantao added a comment.
- Add assertions before accessing getInputs() when assumptions are made about
the number of elements.
- Add missing evaluations of end iterator in the beginning of loops.
- Address othe
tra added a comment.
Few minor nits and suggestions. Other than that I'm OK with the patch.
Comment at: lib/Driver/Action.cpp:156
@@ +155,3 @@
+ // Propagate info to the dependencies.
+ for (unsigned i = 0; i < getInputs().size(); ++i)
+getInputs()[i]->propagateDeviceOfflo
sfantao added a comment.
@tra, any more comments about this patch?
Thanks!
Samuel
http://reviews.llvm.org/D18171
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sfantao updated this revision to Diff 63680.
sfantao added a comment.
- Rebase.
- Remove static function no longer necessary.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
lib/Driver/Action.cpp
lib/
sfantao updated this revision to Diff 62572.
sfantao added a comment.
- Rebase
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
lib/Driver/Action.cpp
lib/Driver/Driver.cpp
lib/Driver/ToolChain.cpp
sfantao updated this revision to Diff 62460.
sfantao added a comment.
- Remove \brief.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
lib/Driver/Action.cpp
lib/Driver/Driver.cpp
lib/Driver/ToolChai
sfantao added a comment.
In http://reviews.llvm.org/D18171#471824, @sfantao wrote:
> Hi Alexey,
>
> Thanks for the review! I addressed your comments in the last diff.
>
> In http://reviews.llvm.org/D18171#471044, @ABataev wrote:
>
> > No '\brief's
>
>
> When you say "no \brief" do you mean I am
sfantao added a comment.
Hi Alexey,
Thanks for the review! I addressed your comments in the last diff.
In http://reviews.llvm.org/D18171#471044, @ABataev wrote:
> No '\brief's
When you say "no \brief" do you mean I am using that where I shouldn't or the
other way around. I am only adding th
sfantao updated this revision to Diff 62435.
sfantao marked 5 inline comments as done.
sfantao added a comment.
- Mark classes with final and fix comments.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
ABataev added a comment.
No '\brief's
Comment at: include/clang/Driver/Action.h:204
@@ -159,1 +203,3 @@
+/// kind to its dependences.
+class OffloadAction : public Action {
virtual void anchor();
'final'
Comment at: include/clang/Driver/Acti
sfantao added a comment.
@tra, any other comments/suggestions about this patch?
Thanks!
Samuel
http://reviews.llvm.org/D18171
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sfantao updated this revision to Diff 62230.
sfantao added a comment.
- Better organize how the offload action is used and add more comments to
document what is going on.
- Rebase.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
incl
sfantao added a comment.
Any more comments on this one?
Thanks!
Samuel
http://reviews.llvm.org/D18171
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sfantao updated this revision to Diff 60573.
sfantao added a comment.
Herald added a subscriber: mehdi_amini.
- Rebase.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
lib/Driver/Action.cpp
lib/Driver
sfantao added a comment.
Hi Art, Justin,
Thanks for the review and feedback! Tried to address your concerns. Let me know
other suggestion you may have.
Thanks again,
Samuel
Comment at: include/clang/Driver/Action.h:95
@@ +94,3 @@
+ /// same host. Therefore, the host offloadi
sfantao updated this revision to Diff 52879.
sfantao marked 15 inline comments as done.
sfantao added a comment.
Address Art, Justin and Eric comments.
http://reviews.llvm.org/D18171
Files:
include/clang/Driver/Action.h
include/clang/Driver/Compilation.h
include/clang/Driver/Driver.h
li
jlebar added a comment.
> @jlebar: Justin, any suggestions on what we can/should do regarding [const
> correctness issues]?
Using "mutable" to work around const-incorrectness should be a last resort.
I've been frustrated by the const-incorrectness of Action as well, but if it's
an issue here
tra added a comment.
Thank you for making these changes. They don't solve all the shortcomings, but
they improve things quite a bit, IMO.
Overall I'm happy with the changes, though use of mutable and changing action
state from const functions may need a look from someone with better C++-fu
ski
sfantao created this revision.
sfantao added reviewers: ABataev, jlebar, tra, echristo, hfinkel.
sfantao added subscribers: caomhin, carlo.bertolli, arpith-jacob, cfe-commits.
This patch replaces the CUDA specific action by a generic offload action. The
offload action may have multiple dependence
30 matches
Mail list logo