SWIFTSIM / SWIFT

Modern astrophysics and cosmology particle-based code. Mirror of gitlab developments at https://gitlab.cosma.dur.ac.uk/swift/swiftsim
http://www.swiftsim.com
GNU Lesser General Public License v3.0
86 stars 57 forks source link

Clock ticks are not always long long ints #18

Closed dacreman closed 2 years ago

dacreman commented 3 years ago

When running make check some tests will fail to build if clock ticks are not recorded as long long ints e.g. on ARM64

test27cells.c:596:61: warning: format specifies type 'long long' but the argument has type 'unsigned long' [-Wformat]
  message("Corner calculations took       : %15lli ticks.", corner_time / runs);
                                            ~~~~~~          ^~~~~~~~~~~~~~~~~~
                                            %15lu
../src/error.h:130:14: note: expanded from macro 'message'
           ##__VA_ARGS__);                                              \
             ^~~~~~~~~~~

This also affects compilation with the --enable-mpiuse-reports option.

Tested on the Isambard system (Thunder X2 processors) with GCC 9.3.0 and ARM 20.0 (based on LLVM 9.0.1) compilers called from Cray wrappers

MatthieuSchaller commented 3 years ago

Thanks.

The clock mechanism comes almost straight from the fftw library. We have small modifications in there when we encounter a platform that is problematic.

On the ARM ThunderX2 in Leicester, we had to configure SWIFT with --enable-armv7a-cntvct or --enable-armv7a-pmccntr (not sure any more which one!).

Would either of these two options work on your system?

LonelyCat124 commented 3 years ago

I think you mean --enable-armv8a-xxx Matthieu. I looked back through my old stuff on this and I think I'd used --enable-armv8a-cntvct last time. I don't think it'd solve this issue though

The easiest solution is probably to just cast this to a long long int. What would be better is probably to convert the times output in these tests from ticks to ms or us.

MatthieuSchaller commented 3 years ago

You are right of course. It's the ARM v8 we are after.

Our configure script tries to detect whether the instruction CNTVCT_EL0 or PMCCNTR_EL0 exists and then defines the corresponding the #ifdef in cycle.h.

The macros are in m4/ax_asm_arm_pmccntr.m4 and m4/ax_asm_arm_cntvct.m4.

If they fail then the code will indeed default to a weird tick definition. That detection might not be working on this platform. We don't provide a mechanism to override the instruction detection however.

If the detection if successful we end up with the tick type defined as an uint64_t, which is different from x86, where it is an unsigned long long.

I agree though that we ought to not print the ticks to screen but rather use our conversion to human-friendly units.

MatthieuSchaller commented 3 years ago

I have pushed a fix to the unit tests as suggested by Aidan above.

At least we won't have a type issue when compiling the unit tests. And hopefully the configuration script can pick up the correct timers.

MatthieuSchaller commented 2 years ago

Fixed in fe9b897e90479051df21bd2c8505c27ce2c50d0c and d078e2dd27837e6ebdde2d6b13e9998af0b7a307.