Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove deprecated Comgr actions #3075

Closed
lamb-j opened this issue Jun 25, 2024 · 15 comments · Fixed by #3107
Closed

Remove deprecated Comgr actions #3075

lamb-j opened this issue Jun 25, 2024 · 15 comments · Fixed by #3107

Comments

@lamb-j
Copy link

lamb-j commented Jun 25, 2024

The AMD_COMGR_ACTION_COMPILE_SOURCE_TO_FATBIN and AMD_COMGR_ACTION_ADD_DEVICE_LIBRARIES actions have been deprecated, and will be removed with the upcoming Comgr 3.0 release.

We should be able to replace them with one of the following:

AMD_COMGR_ACTION_COMPILE_SOURCE_WITH_DEVICE_LIBS_TO_BC

or

AMD_COMGR_ACTION_COMPILE_SOURCE_TO_RELOCATABLE
AMD_COMGR_ACTION_COMPILE_SOURCE_TO_EXECUTABLE

(Although not in the action name, the last two also add the device libraries)

@JehandadKhan
Copy link
Collaborator

@atamazov Can you please take a look when you get a chance ?

@atamazov
Copy link
Contributor

[Notice]

The AMD_COMGR_COMPILE_SOURCE_TO_FATBIN

Typo, it should be AMD_COMGR_ACTION_COMPILE_SOURCE_TO_FATBIN

@atamazov
Copy link
Contributor

@lamb-j

The AMD_COMGR_COMPILE_SOURCE_TO_FATBIN and AMD_COMGR_ACTION_ADD_DEVICE_LIBRARIES actions have been deprecated, and will be removed with the upcoming Comgr 3.0 release.

Are you going to remove these from the amd_comgr_action_kind_t enum or just make them not working?

@atamazov
Copy link
Contributor

@JehandadKhan

The AMD_COMGR_COMPILE_SOURCE_TO_FATBIN

This is only used in comgr::BuildHip(), which is deprecated and not used anymore (we use HIPRTC instead). I am going to remove it anyway.

AMD_COMGR_ACTION_ADD_DEVICE_LIBRARIES

This one is used in two places

  • comgr::BuildHip(), which we do not care of anymore (see above)
  • comgr::BuildOcl(), but only if COMgr version is < 1.7

So it seems like we do not need to make any changes in the library provided that the upcoming changes in COMgr won't break the amd_comgr_action_kind_t enum (see #3075 (comment)).

@lamb-j
Copy link
Author

lamb-j commented Jun 26, 2024

We are planning to remove the following from the enums:

Actions:
AMD_COMGR_ACTION_OPTIMIZE_BC_TO_BC
AMD_COMGR_ACTION_COMPILE_SOURCE_TO_FATBIN
AMD_COMGR_ACTION_ADD_DEVICE_LIBRARIES

Languages:
AMD_COMGR_LANGUAGE_HC

As we're incrementing the major version, our goal is to remove all the functionality previously marked as deprecated

@lamb-j
Copy link
Author

lamb-j commented Jul 8, 2024

@atamazov Any updates?

@atamazov
Copy link
Contributor

atamazov commented Jul 8, 2024

@lamb-j Sorry I was busy with other stuff. The changes described at #3075 (comment) look good to me. I'll make the necessary adaptations in MIOpen ASAP and let you know.

@atamazov
Copy link
Contributor

@lamb-j #3107 contains necessary fixes. Thanks for heads-up.

@atamazov
Copy link
Contributor

@lamb-j
Copy link
Author

lamb-j commented Aug 14, 2024

Need changes in amd-master branch

@lamb-j lamb-j reopened this Aug 14, 2024
@atamazov
Copy link
Contributor

@junliume FYI

Need changes in amd-master branch

@junliume
Copy link
Collaborator

@atamazov @lamb-j is there a pending PR? We need to make changes to develop branch regardless.

@lamb-j
Copy link
Author

lamb-j commented Aug 15, 2024

#3107

We had the above PR against the "develop" branch. Do I need to create similar PRs for the "amd-develop" and "amd-master" branches?

@junliume
Copy link
Collaborator

#3107

We had the above PR against the "develop" branch. Do I need to create similar PRs for the "amd-develop" and "amd-master" branches?

#3107 is already under mainline staging (currently in amd-develop branch), if staging passes it will be merged into mainline amd-master

@lamb-j
Copy link
Author

lamb-j commented Aug 23, 2024

Looks like the patch was promoted to mainline. Thanks!

@lamb-j lamb-j closed this as completed Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants