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.

Sign up for GitHub

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

Jump to bottom

Auto-detect the RISC-V ABI of the compiler and use -mabi= during RISC-V Builds #750

Merged
merged 2 commits into from
Jul 26, 2023
Merged

Auto-detect the RISC-V ABI of the compiler and use -mabi= during RISC-V Builds #750

merged 2 commits into from
Jul 26, 2023

Conversation

leekillough
Copy link
Collaborator

Auto-detect the RISC-V ABI capabilities of the compiler, and use -mabi= to specify it during RISC-V Builds.

Refactor the config/rv*/make_defs.mk makefiles to compute the RISCV_ARCH and RISCV_ABI makefile variables in one place, to be used in several places later.

Generate a build error if there is a 32/64-bit mismatch between the compiler's RISC-V ABI or architecture, and the BLIS configuration selected.

Handle Q, Zicsr, ZiFencei, Zba, Zbb, Zbc, Zbs and Zfh extensions in the RISC-V architecture auto-detection, to be as complete as possible with all standard RISC-V extensions, even if they are not important to BLIS right now.

ZiFencei and Zicsr are not detectable with built-in RISC-V test macros right now. However, ZiFencei is not important for BLIS , because BLIS doesn't have Just-in-time compilation or self-modifying code, and Zicsr is automatically implied by the floating-point extensions, which are required for good performance in BLIS.

Fixes #749
@angsch @apcameron

@leekillough
Auto-detect the RISC-V ABI of the compiler and use -mabi= during RISC…
8c7ea4b 
…-V builds

Generate a build error if there is a 32/64-bit mismatch between the RISC-V ABI
or architecture and the BLIS configuration selected.

Handle Q, Zicsr, ZiFencei, Zba, Zbb, Zbc, Zbs and Zfh extensions in the RISC-V
architecture auto-detection. ZiFencei and Zicsr is not detectable with built-in
RISC-V macros right now.

ZiFencei is not important for BLIS because doesn't it have Just-In-Time
compilation or self-modifying code, and Zicsr is implied by the floating-point
extensions, which are required for good performance in BLIS.
@leekillough leekillough mentioned this pull request Jun 21, 2023
Closed
@apcameron
Copy link

I have successfully tested this PR and can confirm that it compiles successfully on my RISC V Hardware and all the tests pass successfully.

angsch
angsch reviewed Jun 23, 2023
View reviewed changes
frame/base/bli_riscv_detect_arch.h
Comment on lines +208 to +213
#define RISCV_ZIFENCEI
#define RISCV_ZBA
#define RISCV_ZBB
#define RISCV_ZBC
#define RISCV_ZBS
#define RISCV_ZFH
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rm? The identifiers are defined within #if #else above already.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rm? The identifiers are defined within #if #else above already.

That's in an #else clause when __riscv_arch_test is undefined. In that case, we cannot determine whether those extensions exist, so we assume that they don't exist (i.e., we define them as blank).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I didn't see the case distinction due to __riscv_arch_test. Sorry for the noise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For reference, here is the RISC-V C API.

For this PR, here are the relevant sections.

For the order of letters in RISC-V architecture names, please refer to Chapter 27, Section 27.11.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fgvanzee Moved RISC-V autodetection header files to build/detect/riscv/.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @leekillough! 👍🏼

@fgvanzee
Copy link
Member

Is everyone satisfied with the state of this PR?

@fgvanzee fgvanzee merged commit c91b41d into flame:master Jul 26, 2023
2 checks passed
ct-clmsn pushed a commit to ct-clmsn/blis that referenced this pull request Jul 29, 2023
@leekillough @ct-clmsn
Auto-detect the RISC-V ABI of the compiler and use -mabi= during RISC…
d8330ed 
…-V Builds (flame#750)

Details:
- Generate a build error if there is a 32/64-bit mismatch between the 
  RISC-V ABI or architecture and the BLIS configuration selected.
- Handle Q, Zicsr, ZiFencei, Zba, Zbb, Zbc, Zbs and Zfh extensions in 
  the RISC-V architecture auto-detection. ZiFencei and Zicsr is not 
  detectable with built-in RISC-V macros right now.
- ZiFencei is not important for BLIS because doesn't it have 
  Just-In-Time compilation or self-modifying code, and Zicsr is implied 
  by the floating-point extensions, which are required for good 
  performance in BLIS.
- Move RISC-V autodetect header files to build/detect/riscv/.
fgvanzee added a commit that referenced this pull request May 22, 2024
@fgvanzee
Fixed error when using common.mk from testsuite. (#768)
280553d 
Details:
- Commit 2db31e0 (#755) inserted logic into common.mk that attempts to
  preprocess build/detect/android/bionic.h to determine whether the
  __BIONIC__ macro is defined (in which case -lrt should not be included
  in LDFLAGS). However, the path to bionic.h was encoded without regard
  to DIST_PATH, and so utilizing common.mk anywhere that isn't the top-
  level directory (such as in the testsuite directory) resulted in a
  compiler error:

    gcc: error: build/detect/android/bionic.h: No such file or directory
    gcc: fatal error: no input files
    compilation terminated.

  This commit adds a $(DIST_PATH) prefix to the path to bionic.h so that
  it can be located from other applications' Makefiles that use BLIS's
  makefile fragments.
- (cherry picked from commit fa6a9b2)

Set thrcomm timpl_t id inside init functions. (#766)

Details:
- Previously, the timpl_t id being used when a thrcomm_t is being
  initialized was set within the bli_thrcomm_init() dispatch function
  after the timpl_t-specific bli_thrcomm_init_*() function returned. But
  it just occurred to me that each bli_thrcomm_init_*() function already
  intrinsically knows its own timpl_t value. This commit shifts the
  setting of the thrcomm_t.ti field into the corresponding
  bli_thrcomm_init_*() function for each timpl_t type (e.g. single,
  openmp, pthreads, hpx).
- Removed long-deprecated code dating back nearly 10 years.
- Whitespace changes
- Comment updates.
- (cherry picked from 634e532)

Small fixes/improvements to docs/Multithreading.md. (#764)

Details:
- Added reminders that #include "blis.h" must be added to source files
  in order to access BLIS API function prototypes. Thanks to Barry Smith
  for suggesting this improvement.
- Fixed pre-existing typos.
- CREDITS file update.
- (cherry picked from 3cf17b4)

CREDITS file update.

Details:
- Thanks to Igor Zhuravlov for PR #753 (commit 915daaa).
- (cherry picked from dbc7981)

Fix typos in docs + example code comments. (#753)

Details:
- Fixed various typos in API documentation in docs/BLIS*API.md and
  comments in the source code examples within examples/?api/*.c.
- (cherry picked from 915daaa)

Exclude -lrt on Android with Bionic libraries. (#755)

Details:
- Added build/detect/android/bionic.h header to test whether the
  __BIONIC__ cpp macro is defined.
- In common.mk, only add -lrt to LDFLAGS when Bionic is not present.
- CREDITS file update.
- (cherry picked from 2db31e0)

Small fixes to support hpx in the testsuite (#759)

Details:
- Minor changes to test_libblis.c to support hpx.
- (cherry picked from 22ad8c1)

Auto-detect the RISC-V ABI of the compiler and use -mabi= during RISC-V Build
s (#750)

Details:
- Generate a build error if there is a 32/64-bit mismatch between the
  RISC-V ABI or architecture and the BLIS configuration selected.
- Handle Q, Zicsr, ZiFencei, Zba, Zbb, Zbc, Zbs and Zfh extensions in
  the RISC-V architecture auto-detection. ZiFencei and Zicsr is not
  detectable with built-in RISC-V macros right now.
- ZiFencei is not important for BLIS because doesn't it have
  Just-In-Time compilation or self-modifying code, and Zicsr is implied
  by the floating-point extensions, which are required for good
  performance in BLIS.
- Move RISC-V autodetect header files to build/detect/riscv/.
- (cherry picked from c91b41d)

Rewrote regen-symbols.sh (gen-libblis-symbols.sh). (#751)

Details:
- Wrote an alternative to regen-symbols.sh, gen-libblis-symbols.sh,
  that generates a list of exported symbols from the monolithic blis.h
  file rather than peeking inside of the shared object via nm. (This new
  script lives in the 'build' directory and the older script has been
  retired to build/old.) Special thanks to Devin Matthews for authoring
  gen-libblis-symbols.sh.
- Added a 'symbols' target to the top-level Makefile which will refresh
  build/libblis-symbols.def, with supporting changes to common.mk.
- Updates to build/libblis-symbols.def using the new symbol-generating
  script.
- (cherry picked from a0b04e3)

Fix 1m enablement for herk/her2k/syrk/syr2k. (#743)

Details:
- Ever since 28b0982, herk, her2k, syrk, and syr2k have been implemented
  in terms of the gemmt expert API. And since the decision of which
  induced method to use (1m or native) is made *below* the level of the
  expert API, executing any of {herk,her2k,syrk,syr2k} results in BLIS
  checking the enablement status for gemmt.
- This commit applies a band-aid of sorts to this issue by modifying
  bli_l3_ind_oper_get_enable() and bli_l3_ind_oper_set_enable() so that
  any attempts to query or modify the internal enablement status for
  herk, her2k, syrk, or syr2k instead does so for gemmt.
- This solution isn't perfect since, in theory, the user could enable 1m
  for, say, herk but then disable it for syrk, and then be confused when
  herk runs via native execution. But we don't anticipate that users
  modify 1m enablement at the operation level, and so in practice this
  solution is likely fine for now.
- (cherry picked from 89b7863)

add nvhpc compiler support (#719)

Add detection of the NVIDIA nvhpc compiler (`nvc`) in `configure`,
and adjust some warning options in `config.mk`. Currently, no
specific options for `nvc` have been added in the relevant
configurations so it may not be usable without further tweaks.
- (cherry picked from 138de3b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fgvanzee fgvanzee fgvanzee left review comments

@angsch angsch angsch left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RISC-V Target is missing support for -mabi=lp64d based systems
4 participants
@leekillough @apcameron @fgvanzee @angsch