[PATCH] D46472: [HIP] Support offloading by linker script

[Tony Tye via Phabricator via cfe-commits]

t-tye accepted this revision.
t-tye added a comment.
This revision is now accepted and ready to land.

LGTM except for minor suggestions.



================
Comment at: lib/CodeGen/CGCUDANV.cpp:361-373
+  if (IsHIP)
+    FatbinConstantName = ".hip_fatbin";
+  else if (RelocatableDeviceCode)
     // TODO: Figure out how this is called on mac OS!
     FatbinConstantName = "__nv_relfatbin";
   else
     FatbinConstantName =
----------------
Should this all be inside an if (IsHip) so the names can be different for HIP and NV CUDA? Seems HIP should not be using names with VV_CUDA in them. In fact maybe the following IF should be included as well to consilidate the NV CUDA code and HIP code together.


================
Comment at: lib/CodeGen/CGCUDANV.cpp:394-398
   // Fatbin wrapper magic.
   Values.addInt(IntTy, 0x466243b1);
   // Fatbin version.
   Values.addInt(IntTy, 1);
   // Data.
----------------
Should HIP use the same magic value and version number? Perhaps this should also be moved into the IsHip IF.


================
Comment at: lib/CodeGen/CGCUDANV.cpp:438
 
     // void __cudaRegisterLinkedBinary%NVModuleID%(void (*)(void *), void *,
     // void *, void (*)(void **))
----------------
Update comment for HIP


================
Comment at: lib/Driver/ToolChains/CommonArgs.cpp:150
+    // If the current tool chain refers to an OpenMP or HIP offloading host, we
+    // should ignore inputs that refer to OpenMP offloading devices - they will
+    // be embedded according to a proper linker script.
----------------
OpenMP -> OpenMP or HIP


================
Comment at: lib/Driver/ToolChains/CommonArgs.cpp:1321-1322
+
+  // Construct clang-offload-bundler command to bundle object files for
+  // for different GPU archs.
+  ArgStringList BundlerArgs;
----------------
for for -> for


https://reviews.llvm.org/D46472