Skip to content

Conversation

@sunmin89
Copy link
Contributor

@sunmin89 sunmin89 commented Apr 6, 2023

#75 solved
GCC for riscv64 currently support -march,-mabi,-mcpu ,-mtune, -march=riscv64g can be used for almost all the riscv64 machines. I think the latter three flags should be omitted for capacities reason.

The modified source code are tested on x86(wsl2 Ubuntu20.04), aarch64(NanoPC-T4), riscv64 dev board(Nezha D1 and Sifive Unmatched) and riscv64 qemu environment.

ARCH := $(shell uname -p)
was modified to
ARCH := $(shell uname -m)

I'm not sure if this modification would have negative effect.

This PR can not be applied to riscv32 platform.

@gstrauss
Copy link
Collaborator

gstrauss commented Apr 6, 2023

I think the latter three flags should be omitted for capacities reason.

Please explain the above. What do you mean by "for capacities reason"?
Why should your proposed flags be used instead of the existing flags?
What makes gcc -march=native -mtune=native incorrect or suboptimal?
I do not have a RISC-V system, and you likely do, so please explain your proposed changes in detail.

Why change uname -p to uname -m?

-march=riscv64g can be used for almost all the riscv64 machines.

Why "almost all"?

This comment should probably be updated in your PR:
# - Not Supported: RISC-V, IBM Power, etc...

    ifeq ($(ARCH),$(filter $(ARCH),ppc64 ppc64le))
        OPTON += -mcpu=native -mtune=native
    else ifeq ($(ARCH),riscv64)
        OPTON += -march=rv64g -mabi=lp64d
    else
        OPTON += -march=native -mtune=native
    endif

@sunmin89
Copy link
Contributor Author

sunmin89 commented Apr 6, 2023

Hi,

Please explain the above. What do you mean by "for capacities reason"?

The 'capacities' means that my new added feature should apply to all the riscv develop boards.

Why should your proposed flags be used instead of the existing flags?
What makes gcc -march=native -mtune=native incorrect or suboptimal?

The -mabi flag is used to specify 32bit or 64 bit elf, yes, it is better to be specified for riscv64 -mabi=explicitly.

The -march=native or -mtune=native can't be user for riscv64 machine neither.

The -march=rv64g means "RV64IMAFD: RISC-V 64-bit Integer, Multiply and divide, Atomic, Floating point
single and Double precision set support",it is a baseline for modern rv64 cpu design or implementation.

Currently the -mtune flags takes no effection. It should be omitted.You can have a quick look at [1].

Why change uname -p to uname -m?

The uname -p produces 'unknown' in my x86 ubuntu or rv64 debian, while the uname -m produces the correct result.

[1] https://www.sifive.com/blog/all-aboard-part-1-compiler-args
[2] https://www.microsemi.com/document-portal/doc_download/1245378-polarfire-soc-software-development-and-tool-flow-user-guide

'-mabi=lp64d’ means that ‘long’ and pointers are 64-bit (implicitly defining ‘int’ to be 32-bit), and that floating-point values up to 64 bits wide are passed in F registers.

[1] https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Options.html
@gstrauss
Copy link
Collaborator

Thank you for providing additional details. As you noted, RISC-V was not previously supported in unixbench, and gcc does not currently support some flag values, e.g. "native", for -march and -mtune on RISC-V

Please review the suggested code block in my earlier post.

@sunmin89
Copy link
Contributor Author

sunmin89 commented Apr 11, 2023

Hi, @gstrauss

Please review the suggested code block in my earlier post.

Thanks for your words. but could you please supply more details about your earlier post, I failed to find a Pull Request template.
Which file should I modify, the Run script or Makefile ?

@gstrauss
Copy link
Collaborator

In the comment above: #89 (comment) I suggested a different structure to the code change, and I suggested updating the comment to remove the wording which says that RISC-V is not supported.

@sunmin89
Copy link
Contributor Author

sunmin89 commented Apr 11, 2023

In the comment above: #89 (comment) I suggested a different structure to the code change, and I suggested updating the comment to remove the wording which says that RISC-V is not supported.

Hi, @gstrauss I refactored my commit message,and pushed again, please help review the modification.

If you need ssh to riscv64 lab environment , I would try to help you.

@gstrauss
Copy link
Collaborator

It appears that you have overlooked the code in my comment multiple times

    ifeq ($(ARCH),$(filter $(ARCH),ppc64 ppc64le))
        OPTON += -mcpu=native -mtune=native
    else ifeq ($(ARCH),riscv64)
        OPTON += -march=rv64g -mabi=lp64d
    else
        OPTON += -march=native -mtune=native
    endif

I will amend the PR.

@gstrauss gstrauss closed this in b980f9d Apr 11, 2023
@gstrauss
Copy link
Collaborator

With modern version control, editing the outdated changelog is extraneous, so I excluded that from your patch.

Thank you for your contribution.

gstrauss pushed a commit that referenced this pull request Apr 11, 2023
'-mabi=lp64d’ means that ‘long’ and pointers are 64-bit (implicitly defining ‘int’ to be 32-bit), and that floating-point values up to 64 bits wide are passed in F registers.

[1] https://gcc.gnu.org/onlinedocs/gcc/RISC-V-Options.html

Co-authored-by: Glenn Strauss <gstrauss@gluelogic.com>

github: closes #89
@sunmin89
Copy link
Contributor Author

With modern version control, editing the outdated changelog is extraneous, so I excluded that from your patch.

Thank you for your contribution.

You are welcome!

Sorry, I did forget to look at your code more carefully .🙂

Yes, your conditional logic is more clear, thanks for your refactor!

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.

2 participants