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

math/big: different subVV result on pure Go vs assembly implementations #41401

Closed
SparrowLii opened this issue Sep 15, 2020 · 6 comments
Closed
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@SparrowLii
Copy link
Contributor

SparrowLii commented Sep 15, 2020

Look at this test:

func SubVVTest(){
	z:=nat(nil).make(2)
	x:=nat([]Word{1})
	y:=nat([]Word{2})

	c1:=subVV(z,x,y)
	c2:=subVV_g(z,x,y)

	fmt.Println(c1)
	fmt.Println(c2)
}

c1 and c2 will get different results. The reason is that the length of x, y, z is not checked in the assembly code. Some other functions in arith.go have similar problems.
Three solutions:
(1) Remove the length check of x and y in subVV.
(2) Add length check in arith_$GOARCH.s files, though efficiency will become lower.
(3) Remove assembly functions and let the compiler consider these things.

@SparrowLii
Copy link
Contributor Author

@josharian I see you made the corresponding changes in CL164966. How do you think?

@SparrowLii SparrowLii changed the title math/big: inconsistent outputs in arithes math/big: inconsistent outputs of arithes Sep 15, 2020
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 15, 2020
@ALTree ALTree changed the title math/big: inconsistent outputs of arithes math/big: different subVV result on pure Go vs assembly implementations Sep 15, 2020
@josharian
Copy link
Contributor

Does this result in any observable changes when using only the public API? IIRC it is the responsibility of the caller to ensure that the lengths are equal. It is possible we should document that more clearly.

I would love to delete the assembly, but the performance of the Go code is not good enough yet to do that. I got it closer a cycle or two ago, but there's a ways to go.

@SparrowLii
Copy link
Contributor Author

@josharian It does not seem to affect the use of public APIs, and may only cause some troubles for people who want to do things in the math library : ( . It seems that the check on the length of x and y in the function should be removed and then noted in the comment.

@SparrowLii
Copy link
Contributor Author

Should I close this issue or keep it?

@josharian
Copy link
Contributor

I believe that the length checks in the Go code are there to prevent bounds checks in the loop; removing them will make things worse. (Feel free to double-check, though.)

More docs and comments around this would be welcome, to avoid future confusion, if you are so inclined.

@ianlancetaylor
Copy link
Contributor

Personally I think we can close this issue.

@golang golang locked and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants