Updated compiler target architecture #86
Conversation
Aidan-McNay
changed the title
There was a problem hiding this comment.
Looks good to me. @jeffdi , please merge this pull request.
|
@Aidan-McNay : Jeff reports that this is now a problem with the older versions of gcc; apparently while the behavior of including the "zicsr" package was default, there was no specific architecture name associated with it, so that "-march=rv32i_zicsr" just throws an error. @jeffdi : Is this still an issue, or are we dealing with it by insisting on a specific version of gcc? One solution would be to get the gcc version when running the Makefile recipe and set the architecture version accordingly (although I don't know at what version of gcc-riscv this change was made). I believe the Makefile is set up so that the value can be passed as an environment variable, so that's a workaround in either case (old Makefile/new gcc or new Makefile/old gcc). But handling it automatically in the Makefile is the better solution. |
|
@RTimothyEdwards Ok - apologies for the issues. I can take a look into it and get back soon🙂 |
|
Actually, looking at the code base, the Makefile compiler options are not set as variables but hard-coded. So adding something like The |
|
@Aidan-McNay : No need to apologize. I was just hoping you might be willing to work on a more generally-applicable solution (so that I don't have to!). If you can, then great, and you have my thanks. |
|
I created an ARCH variable in the blink Makefile for the
current chipignite firmware. These need to be propagated
to the rest of the repo. It might be better to include a master Makefile
with these parameters set.
…-- Jeff
On Fri, Aug 4, 2023 at 6:22 AM R. Timothy Edwards ***@***.***> wrote:
Actually, looking at the code base, the Makefile compiler options are not
set as variables but hard-coded. So adding something like RISCV_TYPE ?=
rv32i_zicsr would be a minimal effort to provide a workaround in the case
of an incompatible gcc version.
The RISCV_TYPE was used in the caravel verification testbenches to
accommodate the difference in architecture between the original PicoRV32
and the Vex-RISC and should have been carried over into the caravel_board
Makefiles to begin with.
—
Reply to this email directly, view it on GitHub
<#86 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKAZZ5N3TOWZS7I67LFXUEDXTTZQVANCNFSM6AAAAAA2QXKKMY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@jeffdi : You have changed it to |
|
Looked into a bit - it looks as though in older versions, Zicsr was included in the "I" extension. With gcc >= 11.1.0, it was split off and needed to be specified explicitly. https://lore.kernel.org/lkml/[email protected]/t/ Definitely happy to work on it - I like the idea of having a master Makefile that sets variables appropriate for the environment |
|
Sorry - I must not have pushed the correction. Will do now.
…-- Jeff
On Fri, Aug 4, 2023 at 6:30 AM R. Timothy Edwards ***@***.***> wrote:
@jeffdi <https://github.com/jeffdi> : You have changed it to ARCH=rv132
which is not a compiler target. It should be rv32i (or rv32i_zicsr).
—
Reply to this email directly, view it on GitHub
<#86 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKAZZ5PYFLCTG44QZP7NKKTXTT2Q5ANCNFSM6AAAAAA2QXKKMY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Thanks. If you can set that up, I'll look for the PR and push it though.
I'm out of the office next week starting tomorrow, so may not get to review
until I'm back.
…-- Jeff
On Fri, Aug 4, 2023 at 6:31 AM Aidan McNay ***@***.***> wrote:
Looked into a bit - it looks as though in older versions, Zicsr was
included in the "I" extension. With gcc >= 11.1.0, it was split off and
needed to be specified explicitly.
https://github.com/riscv-collab/riscv-gcc/blob/5964b5cd72721186ea2195a7be8d40cfe6554023/gcc/common/config/riscv/riscv-common.c#L409
***@***.***/t/
Definitely happy to work on it - I like the idea of having a master
Makefile that sets variables appropriate for the environment
—
Reply to this email directly, view it on GitHub
<#86 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKAZZ5OP7FMB5UDZT4Q4PBDXTT2RVANCNFSM6AAAAAA2QXKKMY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
FYI - I was not able to update to a version of gcc that recognized the
flag. I'm on an M1 Mac.
…-- Jeff
On Fri, Aug 4, 2023 at 6:15 AM R. Timothy Edwards ***@***.***> wrote:
@Aidan-McNay <https://github.com/Aidan-McNay> : Jeff reports that this is
now a problem with the older versions of gcc; apparently while the
*behavior* of including the "zicsr" package was default, there was no
specific architecture name associated with it, so that "-march=rv32i_zicsr"
just throws an error. @jeffdi <https://github.com/jeffdi> : Is this still
an issue, or are we dealing with it by insisting on a specific version of
gcc?
One solution would be to get the gcc version when running the Makefile
recipe and set the architecture version accordingly (although I don't know
at what version of gcc-riscv this change was made). I believe the Makefile
is set up so that the value can be passed as an environment variable, so
that's a workaround in either case (old Makefile/new gcc or new
Makefile/old gcc). But handling it automatically in the Makefile is the
better solution.
—
Reply to this email directly, view it on GitHub
<#86 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKAZZ5NJOV5YKYFN2RPONLTXTTYWZANCNFSM6AAAAAA2QXKKMY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@jeffdi Just created a PR for this :) (also, not sure about your specific setup, but I'm on an M1 Mac, and I was able to get |
Modified the compiler architecture flag from
-march=rv32ito-march=rv32i_zicsrto work with modern GCC. This should resolve #37 and #87