[PATCH] D18963: PR27216: Only define __ARM_FEATURE_FMA when the target has VFPv4

[silviu.baranga@arm.com via cfe-commits]

sbaranga added inline comments.

================
Comment at: lib/Basic/Targets.cpp:4931
@@ -4931,1 +4930,3 @@
+    if (ArchVersion >= 7 && (CPUProfile != "M" || CPUAttr == "7EM") &&
+        (FPU & VFP4FPU))
       Builder.defineMacro("__ARM_FEATURE_FMA", "1");
----------------
rengolin wrote:
> I think just two checks are necessary, here:
> 
>     (FPU & VFPV4FPU) || (ArchVersion > 7)
> 
> and make sure that the right FPU flag is set from the right cores, plus "+vfp4".
Yes, that should be sufficient.

================
Comment at: test/CodeGen/arm-neon-fma.c:6
@@ -5,2 +5,3 @@
 // RUN:   -ffreestanding \
+// RUN:   -target-feature +vfp4 \
 // RUN:   -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
----------------
rengolin wrote:
> why not change the cpu to a core that has vfp4?
> 
> I know the test is about FMA, not the CPU, but this is a combination that will never occur in the wild...
Sure, good point.

================
Comment at: test/Sema/arm_vfma.c:1
@@ -1,2 +1,2 @@
-// RUN: %clang_cc1 -triple thumbv7s-apple-ios7.0 -target-feature +neon -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple thumbv7s-apple-ios7.0 -target-feature +neon -target-feature +vfp4 -fsyntax-only -verify %s
 #include <arm_neon.h>
----------------
rengolin wrote:
> It's possible that v7 Apple cores always have FMA? I'd make sure of that before forcing the flag here. We don't want to disable it inadvertently.
> 
> @t.p.northover, can you confirm Apple's support for VFP4?
If they do support it and don't have the vfp4 feature, then before this patch clang/llvm wouldn't have emitted a fma/vfma instruction anyway in any circumstances (because the backend will not generate it). The backend would instead legalize it with fmaf() libcalls - but that's not the correct behaviour according to the spec.


http://reviews.llvm.org/D18963