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

The macros to convert trans/notrans etc were not correct for use inline #121

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

abouteiller
Copy link
Contributor

They would cause compilation error when used as functions (e.g. as parameters in a cublass kernel) due to having semicolons etc. Reworded so that they operate as functions (not made them inline because that would cause pulling a bunch of includes when not used.

@abouteiller abouteiller added the bug Something isn't working label Aug 14, 2024
@abouteiller abouteiller self-assigned this Aug 14, 2024
@abouteiller abouteiller requested a review from a team as a code owner August 14, 2024 13:45
@abouteiller abouteiller mentioned this pull request Aug 14, 2024
Copy link
Contributor

@bosilca bosilca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these macros used as functions ? As I commented on the other PR, the compound expressions with values is a GNU extensions, and the correct format is with the ({ ... }).

@abouteiller
Copy link
Contributor Author

abouteiller commented Aug 16, 2024

Where are these macros used as functions ? As I commented on the other PR, the compound expressions with values is a GNU extensions, and the correct format is with the ({ ... }).

They will be in the trsm code (you saw that later), and there will also be more of these uses when we switch to cublas_v2 as passing the literal 'N' will not work anymore.

We do not need the GNU extension for macro-functions (we don't have local variables), so I just used the serial composition operator (,) and that is just regular C.

@abouteiller
Copy link
Contributor Author

Rework:

  • Joseph preferred these macros to be converted to functions (they did look like functions, but had side effects and couldn't actually be used as functions, confusing)
  • HIPBLAS defines HIPBLAS_FILL etc to the same values as CBLAS, which is what dplasma does as well, so there is no need for conversion at all for HIP, removed them.

@bosilca
Copy link
Contributor

bosilca commented Sep 4, 2024

  1. They did not look like functions, you tried to convert them to look like functions.
  2. In general such usage makes the code less debug-friendly, but in this case the functions are short enough.
  3. I prefer to have the HIP code in there as it makes the code more readable less error prone in the long term (by not requiring users to know that DPLASMA and HIP has in 2024 the same values for the constants) and more extendable in the future. Moreover, a good compiler will optimize out unnecessary code anyway.

They are in theory useless (values are cblas based), but for the sake of
symmetry with cublas codebase it is easier to have them.
@QingleiCao
Copy link

What's the problem here? (I remember you told me sometime 😅 @abouteiller )

@abouteiller abouteiller merged commit 17c6c95 into ICLDisco:master Sep 6, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants