Skip to content

[RNG] add RNG armpl backend#634

Merged
andreyfe1 merged 1 commit intouxlfoundation:developfrom
SiPearl:ad/armpl_rng_backend
Mar 5, 2025
Merged

[RNG] add RNG armpl backend#634
andreyfe1 merged 1 commit intouxlfoundation:developfrom
SiPearl:ad/armpl_rng_backend

Conversation

@adegomme
Copy link
Contributor

Description

Add RNG backend for ArmPl
OpenRNG from ARM, included in ArmPl, is a drop in replacement for OneMKL's VSL interface, most of the interface is the same. Some distributions are not yet implemented in OpenRNG (poisson lognormal).
4 tests (integer uniform with a<0 ) fail currently due to an issue in uniform generation for int32, reported to ARM. A check has been added to disable them pending next release

Checklist

All Submissions

  • Do all unit tests pass locally? Attach a log.
    log_rng.txt
    100% tests passed, 0 tests failed out of 154
    28 tests skipped

@adegomme adegomme requested review from a team as code owners February 20, 2025 17:32
Copy link
Contributor

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

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

No concern from my side

add_subdirectory(rocrand)
endif()

if(ENABLE_ARMPL_BACKEND AND UNIX)
Copy link
Contributor

Choose a reason for hiding this comment

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

As it is only supported for Linux, should https://github.com/uxlfoundation/oneMath/blob/develop/cmake/FindARMPL.cmake (or other cmake files) handle the case when ENABLE_ARMPL_BACKEND is specified on Windows to have a clear error message on the configuration stage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually discovered recently that there is an ArmPl version available for Windows/Arm platform, so it may actually work. I don't think I'll be able to try the setup with oneMath anytime soon though, so I'm fine adding a message here.

Copy link
Contributor

@andreyfe1 andreyfe1 left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good to me overall

@@ -0,0 +1,56 @@
/*******************************************************************************
* Copyright 2025 SiPearl
* Copyright 2020-2021 Intel Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please clarify why the Intel's copyright is added?

Copy link
Contributor Author

@adegomme adegomme Feb 25, 2025

Choose a reason for hiding this comment

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

hello, most of these files have been duplicated from oneMKL backend, as implementation is roughly the same (OpenRNG from ArmPl is a drop-in replacement for Intel VSL, using same API. Some calls are not supported, others are added, so minor modifications were still necessary). So initial copyright was retained in these files. I can remove it if necessary, but I see this as derivative, so apache license says to retain them.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. Let us take a closer look to determine whether this copyright is necessary or not

Copy link
Contributor

Choose a reason for hiding this comment

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

We see no issues with that. Let's keep 2 copyrights in this and similar files

Copy link
Contributor

@andreyfe1 andreyfe1 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@andreyfe1 andreyfe1 merged commit c255b1b into uxlfoundation:develop Mar 5, 2025
7 checks passed
@Rbiessy Rbiessy mentioned this pull request Mar 11, 2025
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.

4 participants