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

Hotfix for asan build with flang #619

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

daineAMD
Copy link
Contributor

SWDEV-407304

Basically we shouldn't have a module split between two .f90 files. Disclaimer: I'm in no way a fortran expert.

Changes:

  • add gfortran dep to clients for lapack when built with flang
  • rename hipblas_fortran.f90 -> hipblas_fortran_blas.f90. Contains BLAS functions (for clients), doesn't have hipblas_interface module anymore
  • remove hipblas_interface module from hipblas_fortran_solver.f90
  • create hipblas_fortran_no_solver_module.f90 which includes hipblas_fortran_blas.f90 and has hipblas_interface module. Only built with --nosolver. This could be dealt with a preprocessor macro in hipblas_fortran_module.f90 instead but not exactly sure on best practices/compiler support.
  • create hipblas_fortran_module.f90 which includes hipblas_fortran_blas.f90, hipblas_fortran_solver.f90 and has hipblas_interface module.

We should actually add support for different fortran compilers, this is just ad-hoc and seems to work, but not sure of the future of fortran here.

Copy link
Contributor

@TorreZuk TorreZuk left a comment

Choose a reason for hiding this comment

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

LGTM to separate the overlap this way.
So flang will be used by them but we'll linking gfortran is okay? Did someone change our build to use flang? CI is failing for other reasons so can't see.

@daineAMD
Copy link
Contributor Author

I tested locally by changing the compiler to flang and it seemed to work. The linking is just needed for the reference library, not really sure.
I'm not clear on how they're building hipBLAS in the ticket, they have their own bash script I guess. I'll wait to see if it fixes their problem using their build system (as well as CI with gfortran) before merging this.

@daineAMD daineAMD requested a review from cgmb June 22, 2023 20:57
clients/benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@cgmb cgmb left a comment

Choose a reason for hiding this comment

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

I've prepared updates that fix flang in the build infrastructure. That problem was in their configuration, not in the hipBLAS cmake code, so I recommend against adding workarounds here. (i.e., "add gfortran dep to clients for lapack when built with flang" is unnecessary.) However, the splitting of the interface to fix the no_solver build is still worth doing.

clients/benchmarks/CMakeLists.txt Outdated Show resolved Hide resolved
clients/gtest/CMakeLists.txt Outdated Show resolved Hide resolved
clients/include/hipblas_fortran_module.f90 Outdated Show resolved Hide resolved
clients/include/hipblas_fortran_no_solver_module.f90 Outdated Show resolved Hide resolved
@daineAMD daineAMD merged commit c385c0e into ROCm:release-staging/rocm-rel-5.7 Jul 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants