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

Illegal instruction when AVX instruction set is unsupported #259

Open
chr5tphr opened this issue Jul 31, 2020 · 1 comment
Open

Illegal instruction when AVX instruction set is unsupported #259

chr5tphr opened this issue Jul 31, 2020 · 1 comment
Assignees

Comments

@chr5tphr
Copy link
Contributor

chr5tphr commented Jul 31, 2020

This is related to the problem in #184

When compiling on an architecture that does not support AVX, and then running QuEST, we get an illegal instruction, specifically at:

(gdb) bt
#0  getQuESTDefaultSeedKey (key=0x7fffffff53b0) at .../QuEST/QuEST/src/QuEST_common.c:196
#1  0x00001554fc05284e in seedQuESTDefault () at .../QuEST/QuEST/src/CPU/QuEST_cpu_local.c:306
#2  0x00001554fc0520f7 in createQuESTEnv () at .../QuEST/QuEST/src/CPU/QuEST_cpu_local.c:177
......
(gdb) list 196
195
196         double time_in_mill =
197             (tv.tv_sec) * 1000 + (tv.tv_usec) / 1000 ; // convert tv_sec & tv_usec to millisecond
(gdb) x/i $pc
=> 0x1554fc038282 <getQuESTDefaultSeedKey+95>:  vcvtsi2sd %rax,%xmm0,%xmm0

The culprit instructing specifically in this case is apparently vcvtsi2sd

Notice the missing avx flag in the used cpu:

$ lscpu
Architecture:        x86_64
CPU op-mode(s):      32-bit, 64-bit
Byte Order:          Little Endian
CPU(s):              24
On-line CPU(s) list: 0-23
Thread(s) per core:  2
Core(s) per socket:  6
Socket(s):           2
NUMA node(s):        2
Vendor ID:           GenuineIntel
CPU family:          6
Model:               44
Model name:          Intel(R) Xeon(R) CPU           X5675  @ 3.07GHz
Stepping:            2
CPU MHz:             3189.104
BogoMIPS:            6117.96
Virtualization:      VT-x
L1d cache:           32K
L1i cache:           32K
L2 cache:            256K
L3 cache:            12288K
NUMA node0 CPU(s):   0,2,4,6,8,10,12,14,16,18,20,22
NUMA node1 CPU(s):   1,3,5,7,9,11,13,15,17,19,21,23
Flags:               fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx pdpe1gb rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology nonstop_tsc cpuid aperfmperf pni pclmulqdq dtes64 monitor ds_cpl vmx smx est tm2 ssse3 cx16 xtpr pdcm pcid dca sse4_1 sse4_2 popcnt aes lahf_lm pti ssbd ibrs ibpb stibp tpr_shadow vnmi flexpriority ept vpid dtherm ida arat flush_l1d

A minimal example, given a cpu that does not support avx, can be achived with

// --- test.c ---
#include <stdio.h>
#include <sys/time.h>

int main(unsigned int argc, char **argv) {
    struct timeval tv;
    gettimeofday(&tv, NULL);
    double value = tv.tv_sec * 1000 + tv.tv_usec / 1000;
    printf("%f\n", value);
    return 0;
}

and compiling with gcc -mavx -o test test.c.

The problem as before lies in QuEST/CMakeLists.txt:205.

Do we really need -mavx in any case?
If yes, what for?
Wouldn't it be better to let the user decide which instruction sets to use for possible optimizations?
Additionally, I am not a cmake professional by any means, but shouldn't the flags instead be set later on in target_compile_options(QuEST PRIVATE -Wall)?

@TysonRayJones
Copy link
Member

Hi Christopher,

Sincere apologies for the delay!

I believe AVX is on by default so as to attempt all compatible optimisations without a non-expert user being aware of them (like multithreading). I agree this makes it a bit awkward with incompatible architectures - perhaps one could argue that a user of such an architecture is sufficiently expert so as to be able to disable AVX trivially.
Furthermore, I'm not convinced AVX flags will even make a different on compatible architectures, since we don't explicitly call any AVX instructions - but I haven't yet a moment to confirm this.

I'm not sure if there's a need for mavx to be set in that somewhat inelegant way either, though @aniabrown is the expert on the CMake build.

We'll think more about this, thanks for pointing it out!

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

No branches or pull requests

3 participants