-
Notifications
You must be signed in to change notification settings - Fork 220
Pass correct -march option on RISC-V systems when optarch toolchain option is enabled
#5029
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
Changes from 3 commits
135d880
5ae6951
35baab2
4b4310d
6bf2437
6409794
291b43d
f9035b5
e2f0f7e
494a2e5
1f9ed66
76a3d8e
69f243e
b4f215e
6894973
595bdb2
78c5fd8
76e6881
bb5c5fa
6bdd9f9
f4d81fb
07ab462
21d3bcf
3673cc3
c5fa0a4
48ead25
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -627,6 +627,27 @@ def get_cpu_features(): | |
| return cpu_feat | ||
|
|
||
|
|
||
| def get_isa(): | ||
| """ | ||
| Get supported ISA string | ||
| """ | ||
| isa_string = None | ||
| os_type = get_os_type() | ||
| if os_type == LINUX: | ||
| if is_readable(PROC_CPUINFO_FP): | ||
| cmd = "cat /proc/cpuinfo | grep isa | head -1 | cut -d':' -f2" | ||
| _log.debug("Trying to determine ISA string on Linux via cmd '%s'", cmd) | ||
| res = run_shell_cmd(cmd, in_dry_run=True, hidden=True, fail_on_error=False, with_hooks=False, | ||
| output_file=False, stream_output=False) | ||
| if res.exit_code == EasyBuildExit.SUCCESS: | ||
| isa_string = res.output.strip() | ||
| else: | ||
| _log.debug("%s not found to determine ISA string", PROC_CPUINFO_FP) | ||
| else: | ||
| raise SystemToolsException("Could not determine ISA string (OS: %s)" % os_type) | ||
| return isa_string | ||
|
Comment on lines
652
to
670
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think raising an exception here will cause EB to fail on any non-LINUX systems, as the function calling this needs to always be resolved. Maybe a cleaner solution would be to evaluate this at run-time instead of at the class declaration Also i am wondering if defaulting to an empty string is the right thing to do.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, you are right regarding the exception. I changed it by a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: I changed |
||
|
|
||
|
|
||
| def get_gpu_info(): | ||
| """ | ||
| Get the GPU info | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than relying on a pipeline, can't we just read
/proc/cpuinfoas a file (withread_file), and then use a regular expression to grab theisapart?Also,
grep isais a bit too loose (on an x86_64 system, it matchesmisalignsseinflags, for example).How portable is this across CPU families?
If it's not, we should make this function specific to RISC-V, and return
Noneon non-RISC-V systems?We should also add some tests when introducing a new function like this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added new commits that try to address all the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be risc-v specific (changing the function name to something like
get_isa_riscv)Atleast on my intel CPU the closest thing would be the
flags : ...Details
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Done.