[PATCH] D46476: [HIP] Add action builder for HIP

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl added inline comments.


================
Comment at: lib/Driver/Driver.cpp:2221
+        CudaDeviceActions.clear();
+        for (unsigned I = 0, E = GpuArchList.size(); I != E; ++I) {
+          CudaDeviceActions.push_back(UA);
----------------
tra wrote:
> `for(auto Arch: GpuArchList)`
will do


================
Comment at: lib/Driver/Driver.cpp:2265-2272
+      assert(AssociatedOffloadKind == Action::OFK_Cuda || AssociatedOffloadKind == Action::OFK_HIP);
+
       // We don't need to support CUDA.
-      if (!C.hasOffloadToolChain<Action::OFK_Cuda>())
+      if (AssociatedOffloadKind == Action::OFK_Cuda && !C.hasOffloadToolChain<Action::OFK_Cuda>())
+        return false;
+
+      // We don't need to support HIP.
----------------
tra wrote:
> Please reformat.
will do


================
Comment at: lib/Driver/Driver.cpp:2330-2332
+      for (CudaArch Arch : GpuArchs) {
         GpuArchList.push_back(Arch);
+      }
----------------
tra wrote:
> Single-statement for does not need braces.
will do


================
Comment at: lib/Driver/Driver.cpp:2485-2493
+      // The host only depends on device action in the linking phase, when all
+      // the device images have to be embedded in the host image.
+      if (CurPhase == phases::Link) {
+        DeviceLinkerInputs.resize(CudaDeviceActions.size());
+        auto LI = DeviceLinkerInputs.begin();
+        for (auto *A : CudaDeviceActions) {
+          LI->push_back(A);
----------------
tra wrote:
> I'm not sure I understand what happens here and the comment does not help.
> We appear to add each element of CudaDeviceActions to the action list of each linker input.
> 
> Does the comment mean that *only in linking mode* do we need to add dependency on device actions?
> 
Modified the comment to make it clearer.

We only add dependency on device action at linking phase. HIP embeds device image in host image in host linking phase. Since we need to link all device actions, we cannot create link action here since we have not went through all device actions yet. We just save device actions to  DeviceLinkerInputs and create device link action later in appendLinkDependences, where all device actions have been went through.


https://reviews.llvm.org/D46476