Proposition: changes to hash function tests

[jvdp1]

@wclodius2

Here are some changes:

• rename the directory hash_functions to hash_functions_perf
• mv the directory validation to the directory hash_functions
• add the file test_hash_functions.f90 (aim: same as hash_validity_test.f90

Proposition: for the next step: replace hash_validity_test.f90 by test_hash_function.f90.

It would be nice to get the tests running for all cases, without the need of the 3 different programs (could it be possible that generate_hash_array is called within the test_hash_fucntion.f90 on all platforms?). @awvwgk may have some ideas ;)

Let discuss and try to get the tests running within the CI.

[jvdp1]

@wclodius2 I modified the Makefile.manual such that only one executable is compiled. I also create a module to generate the keys, and link the Cpp to generate the hashs to the main f90 file. So only one executable is needed for run the full test.
The CI with Makefile.manual (with gcc) works fine.

Now it ould be nice to get that into CMake, but my knowledge in CMake is very limited. Any ideas?

[jvdp1]

@wclodius2 the test that includes C/C++ can be compiled with Intel OneAPI 2021 ifort, icc, and icpc.
Which version of Intel compilers did you use?

Edit: it comiples, but fails for nmhash32 as you mentioned it.

[wclodius2]

@jvdp1 so far so good. FWIW I first did a single application version of the validation code before the one currently in the distribution, but the Fortran wrappers for the C/C++ code resulted in additional files. I could a new subdirectory with the single application code.

[wclodius2]

Which version of Intel compilers did you use?

FWIW I used

ifort (IFORT) 2021.2.0 20210228
Copyright (C) 1985-2021 Intel Corporation.  All rights reserved.

However i have upgraded to A Mac Ibook with an M1 chip. While the M1 is nominally "faster" than current Intel processors, ifort would run emulated and I suspect will be several times slower than when I used my Intel based Mac. Given that I remember fort taking over an hour to compile the standard library on my previous machine, I am now reluctant to use ifort to compile the full standard library.

[wclodius2]

Edit: it comiples, but fails for nmhash32 as you mentioned it.

Given the failure with ifort I don't think it can be fully integrated with the standard library. Trying to compile and run it with the rest of the code will cause it to fail on the ifort tests.

[jvdp1]

For both gcc and Intel compilers, the following is used:

#    define NMH_rotl32(x,r) (((x) << (r)) | ((x) >> (32 - (r))))

Actually the preprocessed sources are highly similar between Intel and GCC. The main differences are related to alignment as far as I can understand the code.

It would be good to understand why it fails. i.e., Is it in the C side or in the Fortran side with the Intel compilers?

[wclodius2]

It is my impression that on Intel processors alignment doesn't natter for scalar expressions, but does matter for SIMD or AVE operations. Could

#    define NMH_rotl32(x,r) (((x) << (r)) | ((x) >> (32 - (r))))

expand to SIMD or AVE operation on Intel processors?

It is straight forward, but tedious, to translate my Makefile.validation for my original validation codes, so that it compiles the three applications using the OneAPI compilers. You would also have to compile the stdlib code with ifort. In that case the C/C++ codes are not linked using the ifort "compiler" and any problems would be due to the OneAPI C or C++ compilers.

[jvdp1]

It is my impression that on Intel processors alignment doesn't natter for scalar expressions, but does matter for SIMD or AVE operations. Could

#    define NMH_rotl32(x,r) (((x) << (r)) | ((x) >> (32 - (r))))

expand to SIMD or AVE operation on Intel processors?

@awgwk any idea what could be the problem?

It is straight forward, but tedious, to translate my Makefile.validation for my original validation codes, so that it compiles the three applications using the OneAPI compilers. You would also have to compile the stdlib code with ifort. In that case the C/C++ codes are not linked using the ifort "compiler" and any problems would be due to the OneAPI C or C++ compilers.

I already started here. Actually, in this Makefile, only one excecutable is generated, instead of the 3 aplications. The Makefile.manual at the root of the stdlib directory should define the compilers (either gfortran + gcc + g++ or ifort + icc + icpc).

[wclodius2]

What I was suggesting was that, because the application generate_hash_arrays is compiled using only C/C++ compilers, if that application fails then the problem involves the C/C++ compilers, while if it succeeds the problem is likely with ifort. That cannot be resolved with a single application test as that application would, of necessity, be generated by Fortran, C and C++ compilers. I'll see if ifort can do the stdlib in a reasonable length of time only machine, and, if so, test that hypothesis.

[wclodius2]

I found that ifort 2021.2.0 compiles stdlib in a reasonable time on my M1 Ibook. When I then try to compile my three executable validation code, for the C/C++ generate_hash_arrays I get the error message:

icpc -O3 -L./ generate_hash_arrays.o \
-lc_hash -o generate_hash_arrays
Undefined symbols for architecture x86_64:
  "___builtin_rotateleft32", referenced from:
      _NMHASH32 in generate_hash_arrays.o
      _NMHASH32X in generate_hash_arrays.o
ld: symbol(s) not found for architecture x86_64
make: *** [generate_hash_arrays] Error 1

So at least for this version of the OneAPI compiler suite running on an M1 Ibook if __has_builtin(__builtin_rotateleft32) gives a false positive.

[wclodius2]

Should the compilers be hardwired here, or should they be determined from environmental variables. Hardwiring this doesn't guarantee it will compile if the Library proper is compiled with ifort.

[jvdp1]

Sure, it shouldn't be hardwired. Once the issue is fixed with Intel OneAPI, we can improve this Makefile.

[wclodius2]

I am now convinced that we cannot fix the program's problems with OneAPI. They will have to be fixed by Intel, and the fix will likely only apply to later versions of their compiler suite. In my three executable version of the validation test, the error message implies that the compiler's header files tell the preprocessor that ___builtin_rotateleft32 is defined and hence should be used for the left circular shift, but the linker cannot find the corresponding function in the compiler library.

[wclodius2]

This looks as if you are still going to write the hash files and then read them, while they could be generated and used without the intermediate transfers to and from a binary file.

[jvdp1]

This is indeed the case. My skills in C/C++ are not that high, and I didn't want spend time in something that might not work (especially for Intel OneAPi). If we can get it work, then I think it will be worthwhile that we spend time to improve this approach.

[wclodius2]

Overall the proposed changes are reasonable, but we have to explicitly decide between three choices:

1. Fix the C conditional compilation code for the nmhashes so it never tries to use ___builtin_rotateleft32; or
2. Add constraints in the Makefile so that it will only automatically run the hash validation codes if the GCC compiler suite is being used (I am not an expert on this aspect of Makefiles); or
3. Only allow the hash validation codes to be run manually.

[wclodius2]

I am now convinced that we cannot fix the program's problems with OneAPI. They will have to be fixed by Intel, and the fix will likely only apply to later versions of their compiler suite. In my three executable version of the validation test, the error message implies that the compiler's header files tell the preprocessor that ___builtin_rotateleft32 is defined and hence should be used for the left circular shift, but the linker cannot find the corresponding function in the compiler library.

[wclodius2]

I believe that this will automatically try to run the tests for the hash validation codes, and currently fail if the library is compiled with OneAPI. I believe we have three choices:

1. Fix the C conditional compilation code for the nmhashes so it never tries to use ___builtin_rotateleft32; or
2. Add constraints in the Makefile so that it will only automatically run the hash validation codes if the GCC compiler suite is being used (I am not an expert on this aspect of Makefiles); or
3. Only allow the hash validation codes to be run manually.

[jvdp1]

I am in Hawaii and I believe the meeting starts at 9 AM Honolulu time. If so I will try to attend the meeting, but I have something scheduled a little after 10 and I will have to leave about a half hour after the start of the meeting to make it in time.

Thank you. I guess we can start the meeting with this topic. However, there is no problem if you cannot join it. No worries!

[wclodius2]

It looks like Hawaii does not observe Daylight Savings Time, and the meeting will be at 10 AM my time, so I won't be able to attend.

[jvdp1]

No problems. Do you mind if I introduces it for discussion? Or do you prefer to be there (and then I will not discuss it)?

[wclodius2]

It looks like your changes to stdlib_32_bit_nmhash.f90 are what I would have done. Having almost all the checks pass is a good sign. Because I work on a Mac OS I may be able to address those problems. I am a little concerned that the assignments to run time variables rather than compile time parameters may have a performance impact, but I don't know how to avoid it, and the compiler optimization to remove the run time impact is, I believe, straightforward. FWIW Intel claims that the latest version of OneAPI does not have the problem with finding ___builtin_rotateleft32 that I had.

[wclodius2]

I figure introducing it ASAP is more important than my being there.

[wclodius2]

Inspecting the two check failures:

1. The first failure, @github-actions fpm-deployment / Build (pull_request) Failing after 1m — Build, the last few lines are:
   + gfortran -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fcheck=bounds -fcheck=array-temps -fbacktrace -fcoarray=single -J build/gfortran_2A42023B310FA28D/stdlib -I build/gfortran_2A42023B310FA28D/stdlib build/gfortran_2A42023B310FA28D/stdlib/test_test_hash_functions.f90.o build/gfortran_2A42023B310FA28D/stdlib/libstdlib.a -o build/gfortran_2A42023B310FA28D/test/test_hash_functions
   /usr/bin/ld: build/gfortran_2A42023B310FA28D/stdlib/test_test_hash_functions.f90.o: in function MAIN__': /home/runner/work/stdlib/stdlib/stdlib-fpm-test/test/test_hash_functions.f90:286: undefined reference to generate_all_c_hash'
   collect2: error: ld returned 1 exit status
   Compilation failed for object " test_hash_functions "
   stopping due to failed compilation
   STOP 1
   Error: Process completed with exit code 1.

The problem seems to be the "undefined reference to generate_all_c_hash" at " /home/runner/work/stdlib/stdlib/stdlib-fpm-test/test/test_hash_functions.f90:286". At the moment the only thing I can think of that could cause this message is a failure to include the object file containing generate_all_c_hash in the linkage, but I would expect that to cause problems for the other checks.

2. The last failure, @github-actions CI / intel-build (macos-latest, ifort, icc, icpc) (pull_request) Failing after 38s — intel-build (macos-latest, ifort, icc, icpc) , the problem seems to be in the Configure with CMAKE:
   Run cmake -Wdev -DCMAKE_BUILD_TYPE=Release -DCMAKE_MAXIMUM_RANK:String=4 -DCMAKE_INSTALL_PREFIX=$PWD/_dist -S . -B build
   -- The Fortran compiler identification is Intel 2021.1.0.20201112
   CMake Error at /usr/local/Cellar/cmake/3.22.0/share/cmake/Modules/CMakeDetermineCCompiler.cmake:49 (message):
   Could not find compiler set in environment variable CC:

   icc.
   Call Stack (most recent call first):
   CMakeLists.txt:2 (project)

   CMake Error: CMAKE_C_COMPILER not set, after EnableLanguage
   CMake Error: CMAKE_CXX_COMPILER not set, after EnableLanguage
   -- Configuring incomplete, errors occurred!
   See also "/Users/runner/work/stdlib/stdlib/build/CMakeFiles/CMakeOutput.log".
   Error: Process completed with exit code 1.

This, to me, implies that on the test processor with macOS-latest running ifort the CC (and probably CXX) environmental variables are not set. Setting these environmental variables is probably the responsibility of those generating the build scripts, but there has been no need to set these variables until now. We need to somehow notify them of our requirements. For this one test, but not the others, they need to set CC=ícc and CXX=ipcp.

[awvwgk]

1. C/C++ source code is not collected in the deployment for fpm, also fpm won't pickup any C++ files at the moment for compilation
2. The Intel C/C++ compilers are not installed in the workflow

[jvdp1]

1. C/C++ source code is not collected in the deployment for fpm, also fpm won't pickup any C++ files at the moment for compilation

Currently the C/C++ source code are only for the tests. The stdlib library in itself does not require the C/C++ codes, because @wclodius2 translated them. Does fpm need to compile all the tests?

2. The Intel C/C++ compilers are not installed in the workflow

@awvwgk I modified the CI.yml for Ubuntu, such that Intel C/C++ are installed in the workflow. But my attempts for MacOS were unsuccessful :(

[awvwgk]

Currently the C/C++ source code are only for the tests. The stdlib library in itself does not require the C/C++ codes, because @wclodius2 translated them. Does fpm need to compile all the tests?

I see, still if the test links against the C/C++ source than those must be present and compiled. You can always scrub the validation test files from the deployment by adding them to the prune array:

stdlib/ci/fpm-deployment.sh

Lines 29 to 34 in 1c3a707

	prune=(
	"$destdir/test/test_always_fail.f90"
	"$destdir/test/test_always_skip.f90"
	"$destdir/src/common.f90"
	"$destdir/src/f18estop.f90"
	)

But my attempts for MacOS were unsuccessful :(

I think I have a working install for both Fortran and C/C++ compilers somewhere... maybe this one:

https://github.com/grimme-lab/nlopt-f/blob/696b0e225fe393f1b77006912f09882c25599c48/.github/workflows/build.yml#L193-L210

[wclodius2]

This would be my first time approving a PR. What steps do I need to take to approve @jvdp1's PR for my PR?

[jvdp1]

@wclodius2

During the Monthly call, there was no objection against this approach for testing. @awvwgk repeated some remarks he already mentioned here and gave some explanations about the fpm-deployment CI.

This would be my first time approving a PR. What steps do I need to take to approve @jvdp1's PR for my PR?

Based on that, I will continue to work on the different CIs.When I will be ready, I will update the PR to ready for review such that you can merge it, if you agree with it.

[wclodius2]

@jvdp1 thanks!

[jvdp1]

@wclodius2 Following @awvwgk 's suggestions, I modified the script for the fpm deployment.

I tried to modify the CI.yml for MacOS + Intel, but I was unsuccessfull.

At this stage, I propose to merge this PR into your branch (if you agree with my changes, of course). So. they will appear in the PR in stdlib, and we can discuss there about the last issue (at least for me; i.e. Mac + Intel).

[wclodius2]

@jvdp1 I agree with merging this PR into my branch.

[wclodius2]

I have done the merge.