[Comgr] Add new DataAction flag for device library linking

Previously, some Comgr actions implicitly linked the device libraries,
while others did not.

With this new DataAction option, users can explicitly enforce or disable
device library linking via:

  amd_comgr_action_info_set_device_lib_linking(
                    DataAction ActionInfo,
                    bool ShouldLinkDeviceLibs)

This change impacts the following actions:

  AMD_COMGR_ACTION_COMPILE_SOURCE_TO_BC
  AMD_COMGR_ACTION_COMPILE_SOURCE_TO_EXECUTABLE
  AMD_COMGR_ACTION_COMPILE_SOURCE_TO_RELOCATABLE

  AMD_COMGR_ACTION_CODEGEN_BC_TO_RELOCATABLE
  AMD_COMGR_ACTION_CODEGEN_BC_TO_ASSEMBLY
  AMD_COMGR_ACTION_LINK_RELOCATABLE_TO_EXECUTABLE

If device library linking is required for one of the above actions, it
should be explicitly enforced via the new API. Currently the default
behavior of each action is preserved, but in the future the default
will be changed to NOT link, and linking will only occur if
amd_comgr_action_info_set_device_lib_linking() is used.

Note: In Comgr 4.0, we plan to remove

  AMD_COMGR_COMPILE_SOURCE_WITH_DEVICE_LIBS_TO_BC

In favor of

  amd_comgr_action_info_set_device_lib_linking(ActionInfo, true);
  AMD_COMGR_COMPILE_SOURCE_TO_BC

Change-Id: I46411d1bf01354ac5ad434813df84569956a34d5

Author: lamb-j

amd/comgr/VERSION.txt

@@ -1,4 +1,4 @@
 #COMGR_VERSION_MAJOR
 2
 #COMGR_VERSION_MINOR
-8
+9

amd/comgr/docs/ReleaseNotes.md

@@ -25,6 +25,19 @@ device library from v4 to v5
 (libamd\_comgr.dll -> libamd\_comgr\_X.dll, where X is the major version)
 - oclc\_daz\_opt\_on.bc and oclc\_daz\_opt\_off.bc, and the corresponding
   variable \_\_oclc\_daz\_opt are no longer necessary.
+- Updated default device library linking behavior for several actions.
+  Previously, linking was done for some actions and not others, and not
+  controllable by the user. Now, linking is not done by default, but can
+  optionally be enabled via the
+  amd\_comgr\_action\_info\_set\_device\_lib\_linking() API. Users relying
+  on enabled-by-default behavior should update to use the new API to avoid
+  changes in behavior.
+
+  Note: This does not apply to the \*COMPILE\_SOURCE\_WITH\_DEVICE\_LIBS\_TO\_BC
+  action. This action is not affected by the
+  amd\_comgr\_action\_info\_set\_device\_lib\_linking() API. The new API will
+  allow us to deprecate and remove this action in favor of the
+  \*COMPILE\_SOURCE\_TO\_BC action.
 
 New Features
 ------------
@@ -98,10 +111,14 @@ New APIs
     - For a given executable and ELF virtual address, return a code object
     offset. This API will benifet the ROCm debugger and profilier
 - amd\_comgr\_action\_info\_set\_bundle\_entry\_ids() (v2.8)
-- amd\_comgr\_action\_info\_get\_bundle\_entry\_id_count() (v2.8)
+- amd\_comgr\_action\_info\_get\_bundle\_entry\_id\_count() (v2.8)
 - amd\_comgr\_action\_info\_get\_bundle\_entry\_id() (v2.8)
     - A user can provide a set of bundle entry IDs, which are processed when
     calling the AMD\_COMGR\_UNBUNDLE action
+- amd\_comgr\_action\_info\_set\_device\_lib\_linking() (v2.9)
+    - By setting this ActionInfo property, a user can explicitly dictate if
+    device libraries should be linked for a given action. (Previouly, the
+    action type implicitly determined device library linking).
 
 
 Deprecated APIs

amd/comgr/include/amd_comgr.h.in

@@ -223,11 +223,17 @@ extern "C" {
 #define AMD_COMGR_VERSION_2_7
 
 /**
- * The function was introduced or changed in version 2.7 of the interface
- * and has the symbol version string of ``"@amd_comgr_NAME@_2.7"``.
+ * The function was introduced or changed in version 2.8 of the interface
+ * and has the symbol version string of ``"@amd_comgr_NAME@_2.8"``.
  */
 #define AMD_COMGR_VERSION_2_8
 
+/**
+ * The function was introduced or changed in version 2.9 of the interface
+ * and has the symbol version string of ``"@amd_comgr_NAME@_2.9"``.
+ */
+#define AMD_COMGR_VERSION_2_9
+
 /** @} */
 
 /**
@@ -1496,6 +1502,29 @@ amd_comgr_action_info_get_bundle_entry_id(
   size_t *size,
   char *bundle_entry_id) AMD_COMGR_VERSION_2_8;
 
+/**
+ * @brief Set the device library linking behavior of an action info object.
+ *
+ * Device library linking can be either enforced or omitted for compilation
+ * actions.
+ *
+ * @param[in] action_info A handle to the action info object to be
+ * updated.
+ *
+ * @param[in] should_link_device_libs A boolean that directs the choice to
+ * link the device libraries.
+ *
+ * @retval ::AMD_COMGR_STATUS_SUCCESS The function has
+ * been executed successfully.
+ *
+ * @retval ::AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT @p
+ * action_info is an invalid action info object.
+ */
+amd_comgr_status_t AMD_COMGR_API
+amd_comgr_action_info_set_device_lib_linking(
+  amd_comgr_action_info_t action_info,
+  bool should_link_device_libs) AMD_COMGR_VERSION_2_9;
+
 /**
  * @brief Set the working directory of an action info object.
  *

amd/comgr/src/amdcomgr.def

@@ -49,3 +49,4 @@ amd_comgr_symbol_get_info
 amd_comgr_action_info_set_bundle_entry_ids
 amd_comgr_action_info_get_bundle_entry_id_count
 amd_comgr_action_info_get_bundle_entry_id
+amd_comgr_action_info_set_device_lib_linking

amd/comgr/src/comgr-compiler.cpp

@@ -1149,7 +1149,8 @@ amd_comgr_status_t AMDGPUCompiler::compileToBitcode(bool WithDeviceLibs) {
   Args.push_back("-fshort-wchar");
 #endif
 
-  if (WithDeviceLibs) {
+  // TODO: Deprecate WithDeviceLibs in favor of ActionInfo->ShouldLinkDeviceLibs
+  if (WithDeviceLibs || ActionInfo->ShouldLinkDeviceLibs) {
     if (auto Status = addDeviceLibraries()) {
       return Status;
     }
@@ -1188,8 +1189,12 @@ amd_comgr_status_t AMDGPUCompiler::compileToExecutable() {
   Args.push_back("-fshort-wchar");
 #endif
 
-  if (auto Status = addDeviceLibraries()) {
-    return Status;
+  // TODO: Remove "true" conditional once dependent APIs have included new
+  // new *_set_device_lib_linking API
+  if (ActionInfo->ShouldLinkDeviceLibs || true) {
+    if (auto Status = addDeviceLibraries()) {
+      return Status;
+    }
   }
 
   return processFiles(AMD_COMGR_DATA_KIND_EXECUTABLE, ".so");
@@ -1227,8 +1232,12 @@ amd_comgr_status_t AMDGPUCompiler::compileToRelocatable() {
   Args.push_back("-fshort-wchar");
 #endif
 
-  if (auto Status = addDeviceLibraries()) {
-    return Status;
+  // TODO: Remove "true" conditional once dependent APIs have included new
+  // new *_set_device_lib_linking API
+  if (ActionInfo->ShouldLinkDeviceLibs || true) {
+    if (auto Status = addDeviceLibraries()) {
+      return Status;
+    }
   }
 
   return processFiles(AMD_COMGR_DATA_KIND_RELOCATABLE, ".o");
@@ -1729,6 +1738,12 @@ amd_comgr_status_t AMDGPUCompiler::codeGenBitcodeToRelocatable() {
     }
   }
 
+  if (ActionInfo->ShouldLinkDeviceLibs) {
+    if (auto Status = addDeviceLibraries()) {
+      return Status;
+    }
+  }
+
   Args.push_back("-c");
 
   Args.push_back("-mllvm");
@@ -1748,6 +1763,12 @@ amd_comgr_status_t AMDGPUCompiler::codeGenBitcodeToAssembly() {
     }
   }
 
+  if (ActionInfo->ShouldLinkDeviceLibs) {
+    if (auto Status = addDeviceLibraries()) {
+      return Status;
+    }
+  }
+
   Args.push_back("-S");
 
   return processFiles(AMD_COMGR_DATA_KIND_SOURCE, ".s");
@@ -1768,6 +1789,12 @@ amd_comgr_status_t AMDGPUCompiler::assembleToRelocatable() {
     return Status;
   }
 
+  if (ActionInfo->ShouldLinkDeviceLibs) {
+    if (auto Status = addDeviceLibraries()) {
+      return Status;
+    }
+  }
+
   Args.push_back("-c");
   Args.push_back("-x");
   Args.push_back("assembler");
@@ -1856,6 +1883,12 @@ amd_comgr_status_t AMDGPUCompiler::linkToExecutable() {
     Args.push_back(Inputs.back().c_str());
   }
 
+  if (ActionInfo->ShouldLinkDeviceLibs) {
+    if (auto Status = addDeviceLibraries()) {
+      return Status;
+    }
+  }
+
   amd_comgr_data_t OutputT;
   if (auto Status =
           amd_comgr_create_data(AMD_COMGR_DATA_KIND_EXECUTABLE, &OutputT)) {

amd/comgr/src/comgr.cpp

@@ -1271,6 +1271,22 @@ amd_comgr_status_t AMD_COMGR_API
   return ActionP->setBundleEntryIDs(ArrayRef<const char *>(EntryIDs, Count));
 }
 
+amd_comgr_status_t AMD_COMGR_API
+    // NOLINTNEXTLINE(readability-identifier-naming)
+    amd_comgr_action_info_set_device_lib_linking
+    //
+    (amd_comgr_action_info_t ActionInfo, bool ShouldLinkDeviceLibs) {
+  DataAction *ActionP = DataAction::convert(ActionInfo);
+
+  if (!ActionP) {
+    return AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT;
+  }
+
+  ActionP->ShouldLinkDeviceLibs = ShouldLinkDeviceLibs;
+
+  return AMD_COMGR_STATUS_SUCCESS;
+}
+
 amd_comgr_status_t AMD_COMGR_API
     // NOLINTNEXTLINE(readability-identifier-naming)
     amd_comgr_action_info_set_working_directory_path

amd/comgr/src/comgr.h

@@ -237,6 +237,7 @@ struct DataAction {
   char *Path;
   amd_comgr_language_t Language;
   bool Logging;
+  bool ShouldLinkDeviceLibs = false;
 
   std::vector<std::string> BundleEntryIDs;
 

amd/comgr/src/exportmap.in

@@ -89,3 +89,6 @@ global: amd_comgr_action_info_set_bundle_entry_ids;
         amd_comgr_action_info_get_bundle_entry_id_count;
         amd_comgr_action_info_get_bundle_entry_id;
 } @amd_comgr_NAME@_2.7;
+@amd_comgr_NAME@_2.9 {
+global: amd_comgr_action_info_set_device_lib_linking;
+} @amd_comgr_NAME@_2.8;

amd/comgr/test/compile_minimal_test.c

@@ -137,6 +137,9 @@ int main(int argc, char *argv[]) {
   Status = amd_comgr_create_data_set(&DataSetReloc);
   checkError(Status, "amd_comgr_create_data_set");
 
+  Status = amd_comgr_action_info_set_device_lib_linking(DataAction, true);
+  checkError(Status, "amd_comgr_action_info_set_device_lib_linking");
+
   Status = amd_comgr_do_action(AMD_COMGR_ACTION_CODEGEN_BC_TO_RELOCATABLE,
                                DataAction, DataSetLinked, DataSetReloc);
   checkError(Status, "amd_comgr_do_action");