[PATCH] D18171: [CUDA][OpenMP] Create generic offload action
Samuel Antao via cfe-commits
cfe-commits at lists.llvm.org
Wed Apr 6 18:43:18 PDT 2016
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 offloading kind is a combination of kinds.
+ mutable unsigned OffloadingHostKind;
+ /// \brief Offloading kind of the device.
----------------
tra wrote:
> tra wrote:
> > I assume that by combination you imply that this is a mask of all offloading kinds we intend to use.
> > Perhaps it should be renamed to reflect it better. ActiveOffloadKindMask?
> These fields appears to be part of the Action state yet you want to modify them from const functions which seems wrong to me. Perhaps functions that set these fields should not be const.
That's correct. This is meant to query a host action what are all the programing models used in its dependences.
I am now using `ActiveOffloadKindMask` as you suggested.
================
Comment at: include/clang/Driver/Action.h:95
@@ +94,3 @@
+ /// same host. Therefore, the host offloading kind is a combination of kinds.
+ mutable unsigned OffloadingHostKind;
+ /// \brief Offloading kind of the device.
----------------
sfantao wrote:
> tra wrote:
> > tra wrote:
> > > I assume that by combination you imply that this is a mask of all offloading kinds we intend to use.
> > > Perhaps it should be renamed to reflect it better. ActiveOffloadKindMask?
> > These fields appears to be part of the Action state yet you want to modify them from const functions which seems wrong to me. Perhaps functions that set these fields should not be const.
> That's correct. This is meant to query a host action what are all the programing models used in its dependences.
>
> I am now using `ActiveOffloadKindMask` as you suggested.
Yeah... I was not very happy with these mutable members either... So, after revisiting the issue, the best possible solution was to do all the propagation when the action are appended to the action list. For that I keep track of the offloading kinds employed for a given input and compilation and use that to propagate the information to top-level actions.
I will try to abstract that a little better in the offload handler I am proposing in http://reviews.llvm.org/D18172.
Let me know if you still find issues with the approach I am adopting now.
================
Comment at: include/clang/Driver/Action.h:137
@@ +136,3 @@
+ /// dependences.
+ void propagateDeviceOffloadInfo(OffloadKind OKind, const char *OArch) const;
+ /// \brief Append the host offload info of this action and propagate it to its
----------------
tra wrote:
> propagateXXX implies modification which contradicts 'const' which implies that there will be no state change. Which one is telling the truth?
I refactored the propagation to work on top of non-const actions. So this is not const anymore.
================
Comment at: include/clang/Driver/Action.h:208-215
@@ +207,10 @@
+ private:
+ /// \brief The dependence action.
+ ActionList AL;
+ /// \brief The offloading toolchains that should be used with the action.
+ SmallVector<const ToolChain *, 3> TCL;
+ /// \brief The architectures that should be used with this action.
+ SmallVector<const char *, 3> BAL;
+ /// \brief The offload kind of each dependence.
+ SmallVector<OffloadKind, 3> KL;
+
----------------
tra wrote:
> Are these independent or is it expected that all four are populated/modified in sync? If they all need to be updated at the same time (which is what the code below does), it should be documented. Perhaps these arrays could be converted into vector of structs.
Yes, they are meant to be populated in sync. I'll document that in the comment as you suggest.
The reason I am not using array of structs is that it is more convenient to have the action list separate to forward it to initialize the base class.
================
Comment at: include/clang/Driver/Action.h:220
@@ +219,3 @@
+ /// offload kind.
+ void add(Action *A, const ToolChain *TC, const char *BoundArch,
+ OffloadKind OKind);
----------------
tra wrote:
> Can any of parameters be nullptr?
Only BoundArch can be null. I am changing the signature of `add()` to use reference types for the arguments that must not be null.
================
Comment at: include/clang/Driver/Action.h:234-240
@@ +233,9 @@
+ /// \brief The dependence action.
+ Action *A;
+ /// \brief The offloading toolchain that should be used with the action.
+ const ToolChain *TC;
+ /// \brief The architectures that should be used with this action.
+ const char *BoundArch;
+ /// \brief The offload kind of each dependence.
+ unsigned OffloadKinds;
+
----------------
tra wrote:
> Cosmetic nit: Naming is somewhat inconsistent. Device dependencies use acronyms, but HostDependence uses mix of acronyms and words. I'd use camelcase words in both.
Ok, I am using camelcase in both now. I also noticed I was not using the typedef types in the private members of the dependencies. I fixed that as well.
================
Comment at: include/clang/Driver/Driver.h:422
@@ -424,1 +421,3 @@
+ bool AtTopLevel, bool MultipleArchs,
+ const ToolChain *TC) const;
----------------
tra wrote:
> TC is only used to get triple to construct file name prefix. Perhaps just pass that string explicitly.
Ok, I am just passing the string now.
================
Comment at: lib/Driver/Action.cpp:44
@@ +43,3 @@
+void Action::propagateDeviceOffloadInfo(OffloadKind OKind,
+ const char *OArch) const {
+ // Offload action set its own kinds on their dependences.
----------------
tra wrote:
> Given that these functions change state they probably should not be const.
I fixed that.
================
Comment at: lib/Driver/Action.cpp:103
@@ +102,3 @@
+
+std::string Action::getOffloadingFileNamePrefix(const ToolChain *TC) const {
+ // A file prefix is only generated for device actions and consists of the
----------------
tra wrote:
> There's no need for ToolChain here. A string is all it needs as an input.
Ok, passing the string with the normalized triple now.
================
Comment at: lib/Driver/Action.cpp:208
@@ +207,3 @@
+Action *OffloadAction::getSingleDeviceDependence() const {
+ return (!HostTC && getInputs().size() == 1) ? getInputs().front() : nullptr;
+}
----------------
tra wrote:
> returning nullptr if .size() != 1 looks strange. Perhaps there should be an assert.
This is used when actions get collapsed. In general we can have multiple device dependences, and if so we do not collapse. Therefore this member function was also working as a check.
I'm have changed this to have a 'get' and a 'has' version, and use the assertion in the 'get' version.
================
Comment at: lib/Driver/Driver.cpp:1391-1395
@@ -1357,5 +1390,7 @@
+ const ToolChain *CudaTC =
+ C.getSingleOffloadDeviceToolChain<Action::OFFLOAD_CUDA>();
+
// Build actions for all device inputs.
- assert(C.getSingleOffloadDeviceToolChain<Action::OFFLOAD_CUDA>() &&
- "Missing toolchain for device-side compilation.");
+ assert(CudaTC && "Missing toolchain for device-side compilation.");
ActionList CudaDeviceActions;
----------------
tra wrote:
> Perhaps this should be moved down closer to where it's used. Perhaps even inside of if(PartialCompilation ...)
I moved it right before the if(Partial...) statement, because it is also used after that.
================
Comment at: lib/Driver/Driver.cpp:1464
@@ +1463,3 @@
+ OffloadAction::DeviceDependences DDep;
+ DDep.add(FatbinAction, CudaTC, /*BoundArch=*/nullptr, Action::OFFLOAD_CUDA);
+ return C.MakeAction<OffloadAction>(HDep, DDep);
----------------
tra wrote:
> Is toolchain needed for fatbin action?
Yes, it is required to enquire which link tool should be used for the device action.
================
Comment at: lib/Driver/Driver.cpp:1884
@@ +1883,3 @@
+ }
+ if (auto *DDep = OA->getSingleDeviceDependence())
+ if (isa<T>(DDep)) {
----------------
tra wrote:
> You could fold both ifs into something like this:
>
> ```
> if (auto *DDAP = dyn_cast_or_null<T>(OA->getSingleDeviceDependence()))
> ```
Given that I am adding a new query to check the existence of the Host or single-device action, I am keeping the two ifs separate. `getSingleDeviceDependence ` now does an assertion. Let me know if you prefer me to do this differently.
================
Comment at: lib/Driver/Driver.cpp:2062
@@ +2061,3 @@
+ // If we have a single device action, just return its info.
+ if (!HostAction && OffloadDeviceInputInfos.size() == 1) {
+ return OffloadDeviceInputInfos.back();
----------------
tra wrote:
> It may be worth adding a comment explaining what happens if OffloadDeviceInputInfos.size() != 1.
>
>
>
I am elaborating on that in the comment now. I also got rid of doOnHostDependence here and I use `OA->getHostDependence()` that now contains the assertion.
================
Comment at: lib/Driver/Tools.cpp:3595
@@ -3594,3 +3594,3 @@
CmdArgs.push_back("-aux-triple");
CmdArgs.push_back(Args.MakeArgString(AuxToolChain->getTriple().str()));
CmdArgs.push_back("-fcuda-target-overloads");
----------------
tra wrote:
> All we need is a target triple here. Now that we have device offloading info, perhaps we can bypass AuxToolchain and let offloading info provide host or device triple directly. That would render FIXME above obsolete, IMO.
I removed the use of AuxToolChain. It was being used also for the preprocessor argument. I added some new login in there so that the information is extracted from the toolchains owned by compilation.
Let me know if that is what you had in mind.
http://reviews.llvm.org/D18171
More information about the cfe-commits
mailing list