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

[Determinism] Add deterministic functions #216

Merged
merged 3 commits into from Aug 29, 2018

Conversation

Projects
None yet
3 participants
@shibatch
Copy link
Owner

shibatch commented Aug 17, 2018

This patch adds implementations of deterministic functions.

The SIMD source files(sleefsimd?p.c) are compiled twice for each vector extension, with DETERMINISTIC macro turned on and off.
Renaming by rename*.h is switched according to DETERMINISTIC macro.

When DETERMINISTIC macro is undefined, the function name xsin will be renamed to Sleef_sind2_u35sse2 with renamesse2.h, for example.

If DETERMINISTIC macro is defined, the function name xsin will be renamed to Sleef_cinz_sind2_u35sse2, for example.

iuty* and tester2y* are added in order to test the newly added deterministic functions. As a consequence, time for testing is increased to almost two times.

@shibatch shibatch requested a review from fpetrogalli-arm Aug 17, 2018

@fpetrogalli-arm

This comment has been minimized.

Copy link
Collaborator

fpetrogalli-arm commented Aug 17, 2018

Are there any specific changes I should look for in libsleefgnuabi?

@shibatch

This comment has been minimized.

Copy link
Owner Author

shibatch commented Aug 17, 2018

libsleefgnuabi should be unchanged.

@fpetrogalli-arm

This comment has been minimized.

Copy link
Collaborator

fpetrogalli-arm commented Aug 17, 2018

Thank you. Does it mean that the deterministic versions of the functions are not compiled in libsleefgnuabi?

@shibatch

This comment has been minimized.

Copy link
Owner Author

shibatch commented Aug 17, 2018

At this time, no new functions are added to libsleefgnuabi.
If you want them to be added, please write an issue explaining how they are named.

@fpetrogalli-arm
Copy link
Collaborator

fpetrogalli-arm left a comment

I don't understand how the DETERMINISTIC version are tested with CI in this patch. Could you please explain how this is happening? Or point at CI logs showing that the deterministic tests are executed together with the non deterministic ones?

Please modify the body of the functions according to the comment below.

@@ -389,6 +393,13 @@ static INLINE CONST ddi_t rempi(vdouble a) {
return ret;
}

#if !defined(DETERMINISTIC)

This comment has been minimized.

@fpetrogalli-arm

fpetrogalli-arm Aug 17, 2018

Collaborator

Given that the function is always called "xsin" in C code, I would prefer to have the #if !defined(DETERMINISTIC) switch inside the function definition:

EXPORT CONST vdouble xsin(vdouble d) {
#if !defined(DETERMINISTIC)
// original version
#else // #if !defined(DETERMINISTIC)
// deterministic version
#endif // #if !defined(DETERMINISTIC)
}

Could you please apply this change to all the function definitions of this change-set in which you have introduced DETERMINISTIC behavior?

This is quite important because our downstream version of SLEEF is using the attribute aarch64_vector_pcs [1], and having only one declaration instead of two will make merging easier.

[1] https://developer.arm.com/products/software-development-tools/hpc/arm-compiler-for-hpc/vector-function-abi section 2.1

@fpetrogalli-arm

This comment has been minimized.

Copy link
Collaborator

fpetrogalli-arm commented Aug 17, 2018

At this time, no new functions are added to libsleefgnuabi.
If you want them to be added, please write an issue explaining how they are named.

Thank you for explaining, I don't see a need to add deterministic functions to libsleefgnuabi for now, we can come back to it in the future if needed. I just wanted to make sure that we could keep using libsleefgnuabi in our compiler after the changes in this patch.

@shibatch

This comment has been minimized.

Copy link
Owner Author

shibatch commented Aug 18, 2018

I don't understand how the DETERMINISTIC version are tested with CI in this patch. Could you please explain how this is happening? Or point at CI logs showing that the deterministic tests are executed together with the non deterministic ones?

The iut programs whose name begins with "iuty" are the iut for the deterministic version of functions. By checking the result of testing with iutysse2, for example, you can be sure that the corresponding deterministic functions passes the accuracy and nonnumber tests. I will add bit-identity tests in the next patch.

@fpetrogalli-arm
Copy link
Collaborator

fpetrogalli-arm left a comment

These are just minor changes I am asking you to do, in terms of comments. After these, I will approve the review, but I'd prefer you wait for a final OK from @xoofx before merging it to master. Please add him as a reviewer.

@@ -85,6 +85,20 @@ macro(test_extension SIMD)
add_test_iut(${TARGET_IUT${SIMD}})
list(APPEND IUT_LIST ${TARGET_IUT${SIMD}})

string(CONCAT IUTYNAME "iuty" ${LCSIMD})

This comment has been minimized.

@fpetrogalli-arm

fpetrogalli-arm Aug 21, 2018

Collaborator

Please add a comment in the source explaining the difference between iut and iuty

@@ -100,6 +114,18 @@ macro(test_extension SIMD)
if (MPFR_INCLUDE_DIR)
target_include_directories(${T} PRIVATE ${MPFR_INCLUDE_DIR})
endif()

set(T "tester2y${SCSIMD}${P}")

This comment has been minimized.

@fpetrogalli-arm

fpetrogalli-arm Aug 21, 2018

Collaborator

Same here, make sure the distinction between the old tester2 and tester2y is clear


#if !defined(DETERMINISTIC) && !defined(ENABLE_GNUABI)

#ifdef ENABLE_ALIAS

This comment has been minimized.

@fpetrogalli-arm

fpetrogalli-arm Aug 21, 2018

Collaborator

Can you explain what are these alias for? Please do it in a comment in the code.

@shibatch shibatch requested a review from xoofx Aug 21, 2018

@xoofx

xoofx approved these changes Aug 28, 2018

Copy link
Collaborator

xoofx left a comment

Amazing. On our side, another priority came into the project, so we have postponed deterministic implementation to Q4 (or worse to Q1/2019), but I will try to plug it into our compiler - around September - at least to check that things are running correctly

@shibatch

This comment has been minimized.

Copy link
Owner Author

shibatch commented Aug 28, 2018

@xoofx No problem.
It must be a good thing for the industry to have paradigm shifts frequently. 😃

@shibatch shibatch merged commit 3998463 into master Aug 29, 2018

6 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@shibatch shibatch deleted the Add_deterministic_functions2 branch Aug 29, 2018

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