Updated compiler target architecture

[Aidan-McNay]

Modified the compiler architecture flag from -march=rv32i to -march=rv32i_zicsr to work with modern GCC. This should resolve #37 and #87

[RTimothyEdwards]

@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.

[Aidan-McNay]

@RTimothyEdwards Ok - apologies for the issues. I can take a look into it and get back soon🙂

[RTimothyEdwards]

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.

[RTimothyEdwards]

@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.

[jeffdi]

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 https://github.com/efabless/caravel_board/pull/86#issuecomment-1665601174, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKAZZ5N3TOWZS7I67LFXUEDXTTZQVANCNFSM6AAAAAA2QXKKMY . You are receiving this because you were mentioned.Message ID: @.***>

[RTimothyEdwards]

@jeffdi : You have changed it to ARCH=rv132 which is not a compiler target. It should be rv32i (or rv32i_zicsr).

[Aidan-McNay]

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

https://lore.kernel.org/lkml/c2077e70-98ef-158f-fc82-4ed484f7ee37@iscas.ac.cn/t/

Definitely happy to work on it - I like the idea of having a master Makefile that sets variables appropriate for the environment

[jeffdi]

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 https://github.com/efabless/caravel_board/pull/86#issuecomment-1665612402, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKAZZ5PYFLCTG44QZP7NKKTXTT2Q5ANCNFSM6AAAAAA2QXKKMY . You are receiving this because you were mentioned.Message ID: @.***>

[jeffdi]

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 https://github.com/efabless/caravel_board/pull/86#issuecomment-1665612665, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKAZZ5OP7FMB5UDZT4Q4PBDXTT2RVANCNFSM6AAAAAA2QXKKMY . You are receiving this because you were mentioned.Message ID: @.***>

[jeffdi]

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 https://github.com/efabless/caravel_board/pull/86#issuecomment-1665592299, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKAZZ5NJOV5YKYFN2RPONLTXTTYWZANCNFSM6AAAAAA2QXKKMY . You are receiving this because you were mentioned.Message ID: @.***>

[Aidan-McNay]

@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 riscv64-unknown-elf-gcc v.12.2.0 from Homebrew)