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

Codegen issue under VM - Illegal Instruction #13121

Closed
mdcfrancis opened this issue Sep 14, 2015 · 20 comments
Closed

Codegen issue under VM - Illegal Instruction #13121

mdcfrancis opened this issue Sep 14, 2015 · 20 comments

Comments

@mdcfrancis
Copy link

I created this initially under Gallium

JuliaDebug/Gallium.jl#28

In summary codegen.c inserts avx instructions which are nut supported under Virtual Box VMs

@tkelman
Copy link
Contributor

tkelman commented Sep 14, 2015

You can try building with override MARCH = x86-64 in your Make.user

@mdcfrancis
Copy link
Author

Yes that works, so switching to a generic x86-64 arch ?

@Keno
Copy link
Member

Keno commented Sep 14, 2015

I assume what's happening is that LLVM is identifying features by microarchitecture without taking into account cpuid? I think there is some way to fix that, potentially using llvm::sys::getHostCPUFeatures.

@mdcfrancis
Copy link
Author

For reference this is the first processor (of 4) from /proc/cpuinfo

processor   : 0
vendor_id   : GenuineIntel
cpu family  : 6
model       : 60
model name  : Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz
stepping    : 3
microcode   : 0x19
cpu MHz     : 3591.946
cache size  : 6144 KB
physical id : 0
siblings    : 4
core id     : 0
cpu cores   : 4
apicid      : 0
initial apicid  : 0
fpu     : yes
fpu_exception   : yes
cpuid level : 5
wp      : yes
flags       : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx rdtscp lm constant_tsc rep_good nopl pni ssse3 lahf_lm
bogomips    : 7183.89
clflush size    : 64
cache_alignment : 64
address sizes   : 39 bits physical, 48 bits virtual
power management:

@tkelman
Copy link
Contributor

tkelman commented Sep 14, 2015

I think we already do native somewhere as a default, but LLVM and VirtualBox aren't telling each other the complete truth. There are a bunch of intermediate values you can set somewhere in between generic x86-64 and the haswell that you're running. VirtualBox could probably support some nehalem/sse4 features but I forget how to ask LLVM for the list of flag names it accepts for this.

@mdcfrancis
Copy link
Author

This routine from CommandFlags.h looks like it may be helpful.

00277 static inline std::string getFeaturesStr() {
00278 SubtargetFeatures Features;
00279
00280 // If user asked for the 'native' CPU, we need to autodetect features.
00281 // This is necessary for x86 where the CPU might not support all the
00282 // features the autodetected CPU name lists in the target. For example,
00283 // not all Sandybridge processors support AVX.
00284 if (MCPU == "native") {
00285 StringMap HostFeatures;
00286 if (sys::getHostCPUFeatures(HostFeatures))
00287 for (auto &F : HostFeatures)
00288 Features.AddFeature(F.first(), F.second);
00289 }
00290
00291 for (unsigned i = 0; i != MAttrs.size(); ++i)
00292 Features.AddFeature(MAttrs[i]);
00293
00294 return Features.getString();
00295 }

@mdcfrancis
Copy link
Author

I did not mean to close that

@mdcfrancis
Copy link
Author

So that outputs

-sse4a,-avx512bw,-cx16,-tbm,-adx,-fma4,-avx512vl,-prfchw,-bmi2,-avx512pf,-fsgsbase,-avx,-avx512cd,-rtm,-popcnt,-fma,-bmi,-aes,-rdrnd,-sse4.1,-sse4.2,-avx2,-avx512er,+sse,-lzcnt,-pclmul,-avx512f,-f16c,+ssse3,+mmx,+cmov,-xop,-rdseed,-movbe,-hle,-sha,+sse2,+sse3,-avx512dq

for my CPU

@StefanKarpinski
Copy link
Member

GitHub has the worst placed "Close" button of all time and for some reason refuses to fix it.

@ViralBShah
Copy link
Member

2e10830 shows how to disable some of the CPU features.

@tkelman
Copy link
Contributor

tkelman commented Sep 14, 2015

We aren't using llc in the sysimg build any more. But https://github.com/JuliaLang/julia/blob/master/DISTRIBUTING.md#target-architectures says usr/bin/llc -mattr=help is how to ask LLVM which CPU targets it accepts.

@mdcfrancis
Copy link
Author

I have a possible fix which reads the features directly from llvm, I'm not an llvm expert, but this would seem to put us in a consistent state (is this the same as Arch == native ? )

diff --git a/src/codegen.cpp b/src/codegen.cpp
index 24b154c..fe58393 100644
--- a/src/codegen.cpp
+++ b/src/codegen.cpp
@@ -87,6 +87,8 @@
 #include <llvm/Support/CommandLine.h>
 #include <llvm/Transforms/Utils/Cloning.h>

+#include "llvm//MC/SubtargetFeature.h"
+
 #if defined(_OS_WINDOWS_) && !defined(NOMINMAX)
 #define NOMINMAX
 #endif
@@ -5911,6 +5913,18 @@ static void init_julia_llvm_env(Module *m)
     FPM->doInitialization();
 }

+static inline SmallVector<std::string,10> getTargetFeatures() {
+  StringMap<bool> HostFeatures;
+  SmallVector<std::string,10> attr;
+  if (llvm::sys::getHostCPUFeatures(HostFeatures))
+    for (auto &F : HostFeatures)
+    {
+      std::string att =F.second?F.first().str():std::string("-") + F.first().str();
+      attr.append( 1, att );
+    }
 {
 #if defined(_OS_WINDOWS_) && defined(_CPU_X86_64_)
@@ -5976,19 +5990,6 @@ extern "C" void jl_init_codegen(void)
 #ifdef USE_MCJIT
     jl_mcjmm = new SectionMemoryManager();
 #endif
-    const char *mattr[] = {
-#if defined(_CPU_X86_64_) || defined(_CPU_X86_)
-#ifndef USE_MCJIT
-        // Temporarily disable Haswell BMI2 features due to LLVM bug.
-        "-bmi2", "-avx2",
-#endif
-#ifdef V128_BUG
-        "-avx",
-#endif
-#endif
-        "", // padding to make sure this isn't an empty array (ref #11817)
-    };
-    SmallVector<std::string, 4> MAttrs(mattr, mattr+sizeof(mattr)/sizeof(mattr[0]));
 #ifdef LLVM36
     EngineBuilder eb(std::move(std::unique_ptr<Module>(engine_module)));
 #else
@@ -6016,18 +6017,34 @@ extern "C" void jl_init_codegen(void)
     TheTriple.setEnvironment(Triple::ELF);
 #endif
 #endif
+
+    SmallVector<std::string,10> targetFeatures = getTargetFeatures();
+
+#if defined(_CPU_X86_64_) || defined(_CPU_X86_)
+#ifndef USE_MCJIT
+    // Temporarily disable Haswell BMI2 features due to LLVM bug.
+    targetFeatures.append( 1, "-bmi2")
+    targetFeatures.append( 1, "-avx2")
+#endif
+#ifdef V128_BUG
+    targetFeatures.append( 1, "-avx")
+#endif
+#endif
+
+#ifdef _CPU_ARM_
+    // Check if this is required when you have read the features directly from the processor
+    targetFeatures.append(1, "+vfp2"); // the processors that don't have VFP are old and (hopefully) rare. this affects the platform calling convent
+#endif
+
     std::string TheCPU = strcmp(jl_options.cpu_target,"native") ? jl_options.cpu_target : sys::getHostCPUName();
     if (TheCPU.empty() || TheCPU == "generic") {
         jl_printf(JL_STDERR, "WARNING: unable to determine host cpu name.\n");
-#ifdef _CPU_ARM_
-        MAttrs.append(1, "+vfp2"); // the processors that don't have VFP are old and (hopefully) rare. this affects the platform calling convention.
-#endif
     }
     TargetMachine *targetMachine = eb.selectTarget(
             TheTriple,
             "",
             TheCPU,
-            MAttrs);
+            targetFeatures);
     assert(targetMachine && "Failed to select target machine -"
                             " Is the LLVM backend for this CPU enabled?");
     jl_TargetMachine = targetMachine->getTarget().createTargetMachine(

@mdcfrancis
Copy link
Author

I can put that out there as a pull request if people want me to?

@vtjnash
Copy link
Member

vtjnash commented Sep 14, 2015

Alternatively, you could setup VirtualBox to stop lying about its processor :).

If we go with this approach (which looks fairly sane to me), we should only use it if the user specifies "native" as the cpu_target (or does not specify a cpu_target). It would also be good to expose this information through jl_get_cpu_name and versioninfo.

@mdcfrancis
Copy link
Author

I was surprised to find that the kf/gallium branch is compiling with c++11 vs master which c98. Which is the standard for the project.

@yuyichao
Copy link
Contributor

latest llvm (>=3.5?) requires c++11.

@mdcfrancis
Copy link
Author

Makes sense, at what point is master going to be reved to c++11 ?

@yuyichao
Copy link
Contributor

The master is already using c++11 for newer llvm versions. It will do it by default/require it whenever we use llvm 3.5+ by default.

@mdcfrancis
Copy link
Author

I created a pull request which fixes the issue, though I'm not convinced that we are doing the right thing when we pass the CPU type to selectTarget.

#13142

@vtjnash
Copy link
Member

vtjnash commented Mar 26, 2016

fixed in #13142

@vtjnash vtjnash closed this as completed Mar 26, 2016
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

7 participants