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.

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

proposal: cmd/go: add GOMIPS64=r2 for mips64r2 code generation #35008

Open
mengzhuo opened this issue Oct 19, 2019 · 11 comments
Labels
Milestone

Comments

@mengzhuo
Copy link
Contributor

@mengzhuo mengzhuo commented Oct 19, 2019

According to the wiki the minimal requirement of MIPS64X is MIPS III.

MIPS III is introduced in 1991 the cpu is R4000 (which clock is only 200MHz) and MIPS64r1 adds instructions like CMOV, CLZ, ROR for better performance but we can't use them due to the minimal requirement.

I propose add a GOMIPS64REV flag for compiler. We having GOMIPS64 flag is for hardware float (hardfloat/softfloat), i.e. GOMIPS64=hardfloat now. For the backward-compatible, we don't change it and add a new flag GOMIPS64REV.

Further more, MIPS had announced MIPS64r6 which deleted some instructions like "likely branch" and "unaligned read/load" which is another reason we should to add GOMIPS64REV flag if we want to support MIPS64r6 at compile time in the future.

Therefore the variable of "GOMIPS64REV" will be one of

mipsiii
mips64r2
mips64r6

mipsiii is same as current minimal support.
No base instructions were introduced between mips64r2 - mips64r5.

Related topic:
#31265
https://groups.google.com/forum/#!topic/golang-dev/utpaIeO9lx4

@cherrymui @randall77

@gopherbot gopherbot added this to the Proposal milestone Oct 19, 2019
@gopherbot gopherbot added the Proposal label Oct 19, 2019
@mengzhuo mengzhuo changed the title proposal: cmd/go: adding GOMIPS64=r* for MIPS64r* proposal: cmd/go: adding GOMIPS64REV for MIPS64r* Oct 19, 2019
@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Oct 19, 2019

If we want to do this, I think we should just use the GOMIPS64 environment variable, with a comma-separated list, with default values if not all values are provided. For example,

  • GOMIPS64=softfloat,r2 means softfloat+MIPS64r2
  • GOMIPS64=softfloat means softfloat + minimal ISA (default ISA)
  • GOMIPS64=r2 means hardfloat (default FP mode) + MIPS64r2
  • GOMIPS64="" means hardfloat + minimal ISA (default for both)

I think this is already proposed when we introduced the hard/softfloat mode. There is really no need to write "mips(64)" in the setting. Maybe we can drop the "r" as well (just GOMIPS64=2, more like GOARM)?

That said, if there are only a few instructions, I think we may want to try run-time CPU feature detection and see how that goes.

@beenshi

This comment has been minimized.

Copy link

@beenshi beenshi commented Oct 19, 2019

I support adding all to the GOMIPS64 as a comma separated list, than a new env var. But I do not support runtime check. Just like I proposed in #29373

@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented Oct 19, 2019

To clarify, my personal vote would be not to do this. My comment above is under the assumption that we really want to do this.

For one, the MIPS port is less well tested already, and adding more flavors would make testing even harder. We need softfloat/hardfloat as there is really no way around, and it is well tested as the implementation is mostly portable. It is different for a few new instructions. I'm not sure the benefit overweighs the cost.

@mengzhuo

This comment has been minimized.

Copy link
Contributor Author

@mengzhuo mengzhuo commented Oct 26, 2019

I agree with GOMIPS64=hardfloat,r2.

After dig into MIPS64 instruction set manual

I found we can add these instructions into compiler along with CLZ, CLO

SEB Sign-Extend Byte 
SEH Sign-Extend Halftword 
DEXT Doubleword Extract Bit Field 
DEXTM  Doubleword Extract Bit Field Middle 
DEXTU  Doubleword Extract Bit Field Upper 
DINS  Doubleword Insert Bit Field 
DINSM  Doubleword Insert Bit Field Middle 
DINSU  Doubleword Insert Bit Field Upper
DSBH  Doubleword Swap Bytes Within Halfwords
DSHD  Doubleword Swap Halfwords Within Doublewords
DROTR Doubleword Rotate Right
DROTR32 Doubleword Rotate Right Plus 32 
DROTRV Doubleword Rotate Right Variable
ROTR Rotate Word Right
ROTRV Rotate Word Right Variable
MADD.fmt Floating Point Multiply Add
MSUB fmt Floating Point Multiply Subtract
NMADD fmt Floating Point Negative Multiply Add
NMSUB fmt Floating Point Negative Multiply Subtract
RECIP fmt Reciprocal Approximation
RSQRT fmt Reciprocal Square Root Approximation

As matter of tests, we can setup mips64enc.s like arm64 did.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 27, 2019

Change https://golang.org/cl/203441 mentions this issue: cmd/compile, cmd/go: add mips64r2 flag in GOMIPS64

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 28, 2019

Change https://golang.org/cl/203443 mentions this issue: cmd/compile, cmd/go: add mips64r2 flag in GOMIPS64

@martisch

This comment has been minimized.

Copy link
Member

@martisch martisch commented Oct 28, 2019

I agree with cherrymui@ in that it is unclear (at least to me) whether the added complexity is worth the benefit.

Will there be builders for the new combinations if we do not have any?
Do we have (or even support) codegen tests for these different variants?

It would also be nice to know what the performance benefits are vs not doing this or doing runtime detection of the features (if that is possible).

If pre r2 is not widely used an alternative might be to just make r2 the new minimal requirement.

@mengzhuo

This comment has been minimized.

Copy link
Contributor Author

@mengzhuo mengzhuo commented Oct 28, 2019

I agree with cherrymui@ in that it is unclear (at least to me) whether the added complexity is worth the benefit.

Will there be builders for the new combinations if we do not have any?
Do we have (or even support) codegen tests for these different variants?

We don't have codegen test yet. I will work on it soon.

It would also be nice to know what the performance benefits are vs not doing this or doing runtime detection of the features (if that is possible).

That is impossible. I've asked about Linux kernel maintainer to add ISA to uapi/hwcap but they insisted that's a compiler issue.
However they do offer DSP, MSA, CRC32 feature detection.

If pre r2 is not widely used an alternative might be to just make r2 the new minimal requirement.

I would love to set r2 as minimal requirement same as mips32x do.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 30, 2019

Change https://golang.org/cl/204297 mentions this issue: cmd/asm: add encode testes for MIPS64x

gopherbot pushed a commit that referenced this issue Nov 6, 2019
This CL adds basic encode test for mips64x and
most of the instructions are cross checked with 'gas'

Update #35008

Change-Id: I18bb524897aa745bfe23db43fcbb44c3b009463c
Reviewed-on: https://go-review.googlesource.com/c/go/+/204297
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@rsc rsc changed the title proposal: cmd/go: adding GOMIPS64REV for MIPS64r* proposal: cmd/go: add GOMIPS64=r2 for mips64r2 code generation Nov 6, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 6, 2019

It seems like the alternative is to make r2 the minimum requirement for mips64.
Does anyone have any data about what mips64 chips are typically used today and whether they have r2 or not?

@mengzhuo

This comment has been minimized.

Copy link
Contributor Author

@mengzhuo mengzhuo commented Nov 7, 2019

I had send email to MIPS company and ask for more information.
AFAIK, Loongson 3A1000 the first MIPS64r2 in Loongson series which is released in 2009.

Also Debian require of MIPS64r2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.