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

================
Comment at: clang/lib/Driver/Driver.cpp:2725-2726
         for (unsigned I = 0, E = GpuArchList.size(); I != E; ++I) {
           // Create a link action to link device IR with device library
           // and generate ISA.
+          CudaDeviceActions[I] = C.getDriver().ConstructPhaseAction(
----------------
tra wrote:
> The comment about "create link action" should probably be moved down below to 
> where the link action is constructed now.
> 
will do


================
Comment at: clang/lib/Driver/Driver.cpp:2727-2732
+          CudaDeviceActions[I] = C.getDriver().ConstructPhaseAction(
+              C, Args, phases::Backend, CudaDeviceActions[I],
+              AssociatedOffloadKind);
+          CudaDeviceActions[I] = C.getDriver().ConstructPhaseAction(
+              C, Args, phases::Assemble, CudaDeviceActions[I],
+              AssociatedOffloadKind);
----------------
tra wrote:
> Looks like we're chaining backend/assembly actions here. but it's not easy to 
> spot that we use `CudaDeviceActions[I]`  as an intermediate value. At the 
> first glance it looked like a copy/paste error writing to 
> `CudaDeviceActions[I]` multiple times.
> 
> It would be easier to see what's going on if the code was structured like 
> this:
> ```
> BackendAction = Construct(... CudaDeviceActions[I]);
> AssembleAction  = Construct(... BackendAction);
> AL.push_back(AssembleAction)
> CudaDeviceActions[I] = C.MakeAction<LinkJobAction>(AL);
> ```
> 
will do


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81627/new/

https://reviews.llvm.org/D81627



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to