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

Implement Math.multiplyHigh for Long #3480

Merged
merged 2 commits into from Oct 4, 2023

Conversation

jspenger
Copy link
Contributor

This is a draft PR towards implementing the Math.multiplyHigh method for Long #2473. Currently missing are some tests, and will also look over the method implementation with fresh eyes next week. This was done as part of the Scala-Spree in Madrid.

The algorithm used here is based on the one presented in Hacker's Delight (Algorithm 8.2).

@plokhotnyuk
Copy link

Could it be done more efficiently as mapping to C functions, like here?

@armanbilge
Copy link
Member

Could it be done more efficiently

We also discussed on Discord once if it can be implemented more efficiently by using the LLVM intrinsics with i128 integers.

https://discord.com/channels/632150470000902164/635668881951686686/1140736146326949900


With that said, this PR is an improvement over the status quo, and more optimizations can be follow-up.

@LeeTibbert
Copy link
Contributor

@tanishiking

I do not mean to sound harsh here but to reinforce community standards.
Scala Native implementors can not look at OpenJDK code. They can look at JDK API documentation.

Posting a URL makes it too easy, especially for first time
contributors, to inadvertently step over the line. I suggest deleting that post and avoiding such.

@LeeTibbert
Copy link
Contributor

@jonasspenger

Nice to see this contribution.

I suggest that you not be concerned or spend time on the "multiarch" linux-arm-64 CI pidfd_open failures.
I am chasing and discuss similar ones in PR #3477. They appear to have nothing to do with the changes
of either this WIP PR or #3477/#3478.

I suspect something went bump in the night in mainline some where around Sept 6 or 7. I hope to
be chasing that this weekend.

I will monitor this PR to see if any other errors look related. My hope is to save you time & anxiety, so
that you can focus on your problem-at-hand (and not get too discouraged by the CI process).

Please let me know if you have questions or concerns.

@WojciechMazur
Copy link
Contributor

Thank you for this contribution. The only thing that is missing is probably some test, you create them in unit-tests/shared/src/test/scala. After doing that you can test it using testsJVM3/test to ensure it executes correctly on the JVM and later test it on native using tests3/test

Note: if testing with Scala 2 replace the 3 suffix with 2_12 or 2_13

@jspenger
Copy link
Contributor Author

jspenger commented Sep 25, 2023

Thanks for all the helpful comments! And apologies for being slow with the update.

I've read now the contributions guidelines, apologies for the hickup with regards to the OpenJDK Code; once seen it cannot be unseen.

I've cleaned up the code and tested it. However, as mentioned above by others, the possibility of implementing with LLVM intrinsics or similar seems to be the most promising solution. For these reasons I think it might be better to go with the LLVM solution. I have posted an update anyways, just to finalize the work that was started.

Summary

I've fixed the code, and added some test cases. Let me know what is appropriate. It runs on my machine with the Scala 3 and 2 tests for JVM and Native.

Some further comments:

It was unclear if to use @inline def or @alwaysinline def as methods seemed to differ on this in the Math object.

@WojciechMazur
Copy link
Contributor

To allow for usage of LLVM intrinsics, then we probably would first need to introduce concept of Int128 (possibly in both primitive and boxed form) which we later would be able to use in LLVM multiply + truncate to int64. However, it might take a bit of time of provide that, so let's start with implementing it using the provided algorithm.

@@ -275,4 +275,57 @@ class MathTest {
}
}

@Test def multiplyHighTests(): Unit = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI fails becouse JDK 8 does not define this method. Math.multipleHight was added in JDK 9, so we need to put the test in this file


The require-jdkX ensures that given test is only compiled when using JDK X or later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hint! I've moved the test to the linked file. Hopefully all checks are successful.

@jspenger
Copy link
Contributor Author

jspenger commented Oct 3, 2023

I also tested on some random numbers as a sanity check, but the test might be too heavy for the CI, so I'll just paste it here instead. Perhaps someone will find it useful.

import scala.util.Random
import scala.math.BigInt
for (i <- 0 until 1000000) {
  val a = BigInt(Random.nextLong())
  val b = BigInt(Random.nextLong())
  val result = Math.multiplyHigh(a.toLong, b.toLong)
  val ab = a * b
  val expected = (ab >> 64).longValue
  assertTrue(
    s"Math.multiplyHigh(${a.toLong}, ${b.toLong}) result: ${result} != expected: ${expected}",
    Math.multiplyHigh(a.toLong, b.toLong) == expected
  )
}

@WojciechMazur WojciechMazur marked this pull request as ready for review October 4, 2023 11:30
@WojciechMazur WojciechMazur changed the title WIP: Implement Math.multiplyHigh for Long Implement Math.multiplyHigh for Long Oct 4, 2023
@WojciechMazur WojciechMazur merged commit 6c89cde into scala-native:main Oct 4, 2023
76 checks passed
WojciechMazur pushed a commit to WojciechMazur/scala-native that referenced this pull request Oct 13, 2023
* Created Math multiplyHigh method
* Move multiplyHigh tests to JDK9 tests dir

(cherry picked from commit 6c89cde)
WojciechMazur pushed a commit that referenced this pull request Oct 13, 2023
* Created Math multiplyHigh method
* Move multiplyHigh tests to JDK9 tests dir

(cherry picked from commit 6c89cde)
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.

None yet

5 participants