ROCm / ROCm-CompilerSupport

The compiler support repository provides various Lightning Compiler related services.
47 stars 31 forks source link

Exit status aliasing in API #10

Closed stuartarchibald closed 1 year ago

stuartarchibald commented 5 years ago

I think exit status AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT ends up being aliased in the API logic. For example, specifying a nonsense action kind to amd_comgr_do_action returns AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT, but so does specifying a valid action kind to amd_comgr_do_action but with nonsense input, for example AMD_COMGR_ACTION_SOURCE_TO_PREPROCESSOR with an unsupported language set in the action info.

scott-linder commented 5 years ago

I believe the common thread in all of the APIs is that AMD_COMGR_STATUS_ERROR_INVALID_ARGUMENT indicates an error on the caller's part, whether it be an invalid action kind, invalid language, etc.

In the docs for amd_comgr_do_action it indicates a few cases where it will be returned, then delegates to the description of each action_kind for the rest.

For AMD_COMGR_ACTION_SOURCE_TO_PREPROCESSOR it indicates it can be returned "if isa name or language is not set in @p info." While this doesn't specifically indicate that specifying a valid-but-unsupported language will return this status, I think it is reasonable, and e.g. AMD_COMGR_ACTION_ADD_PRECOMPILED_HEADERS does indicate this explicitly.

I would like to get input from the original designer of the API, @t-tye, but he just started a vacation. I can see the argument that, strictly speaking, the error code should indicate whether the formal parameters to the function are all generically valid, not that they are correct/supported/sensible in context.

lamb-j commented 1 year ago

It could be nice to have two different error enums for invalid action and invalid action argument, but at this point the disruption it would cause may not be worth the effort. Instead, I've added some clarity to the documentation:

https://github.com/RadeonOpenCompute/ROCm-CompilerSupport/commit/a11a02572bc06a4e3a9b6cc0c9b92ba2aec41f72