Define version only once in CMakeLists

[barracuda156]

UPD. I dropped PowerPC-related stuff from here and a fix to headers in favor of other PR, retaining only a trivial CMakeLists improvement.

[biergaizi]

First of all, I'm not the project developer, but just a spare-time contributor, so I don't represent the project officially. The following is my personal opinion.

TL; DR: I'm working on a competing patch that would make this Pull Request obsolete.

Don't use uint

First of all,

#ifdef __APPLE__
#include <sys/types.h> // uint
#endif

This is not the correct fix. The problem is introduced because its original contributor was unfamiliar with ISO C and POSIX, so the non-standard type uint was incorrectly used. Instead of including more non-standard headers, the correct fix is replacing the usage of uint with uint32, which is defined by ISO C in stdint.h.

CPU architecture detection code

In my opinion, the current CPU architecture detection code in CMake is the wrong way of doing things. Instead, I have a different plan that will delete most of the detection code entirely. So it's not necessary to merge this Pull Request.

Why do we need CPU detection in the first place? Because of two things:

1. The simulation engine wants to disable denormalized numbers using x86-specific code. So when targeting other architectures, this logic must be disabled. So CMake passes the macro -DSSE_CORRECT_DENORMALS on all non-x86 system to disable this part of the code.
2. Little-Endian IBM POWER can use the same intrinsic functions for x86 by passing the macro -DNO_WARN_X86_INTRINSICS.

To solve the first problem, the original IBM POWER contributor added a hardcoded check for x86 and IBM Power, and later it was extended by other contributors to ARM. But in fact it's unnecessary, it does nothing but reducing the code portability by limiting supported CPUs to a hardcoded list. For example, fundamentally there's nothing to stop the codebase from working on RISC-V, but because nobody has allowlisted the CPU yet, it fails the check. In the future, as new users arrive, the check will be longer and longer for no good reason.

The original contributor introduced this macro because of their unfamiliarity with the codebase. In fact, "the disabling denormal number" logic involves only 3 lines of code, and is only used in two places in the code. Thus, a better solution is extracting this piece of logic into a small function, called disableDenormal(). Then one can use #ifdef checks to selectively compile a 3-line function or a noop function. This also allows better portability, because one can extend the support of disabling denormal numbers to other architecture by changing one function only (though I'm not sure whether the common desktop and server ARM CPUs have any penalty for denormals).

The next problem: passing -DNO_WARN_X86_INTRINSICS for IBM POWER, has a similar problem. Instead of adding a global check, a #ifdef check inside the "SSE" engine is perhaps more reasonable.

[biergaizi]

I've opened PR #146 and PR #147 to fix both problems "properly" as a replacement of this Pull Request. You're invited to test it on PPC and macOS (note that -maltivec flag must be manually set in CXXFLAGS if desired, the openEMS default build doesn't set any flag on any architecture by default, so I feel that hardcoding -maltivec just for PPC isn't appropriate).

[barracuda156]

@biergaizi Thank you, I will look into your code today. Generally I agree with your reasoning.

To be clear, my aim was merely to make the project compile with minimal sensible fixes so that it can be used by ppc users (riscv is not yet supported in MacPorts, one would have to fix a lot more stuff prior to building this one). So I did not try to redesign anything.

[barracuda156]

P. S. I will close this PR in favor of new ones once confirm that the build on ppc works.

[thliebig]

Whats the status here?

[barracuda156]

Whats the status here?

@thliebig Ah, this should be merged, I guess.
(I will recheck the source once get to the machine just in case.)

[thliebig]

I think this line 26 can be just removed?? It is already set in line 18? Am I missing something?