PR27216: Only define __ARM_FEATURE_FMA when the target has VFPv4

[sbaranga]

Summary

According to the ACLE spec, "__ARM_FEATURE_FMA is defined to 1 if
the hardware floating-point architecture supports fused floating-point
multiply-accumulate".

This changes clang's behaviour from emitting this macro for v7-A and v7-R
cores to only emitting it when the target has VFPv4 (and therefore support
for the floating point multiply-accumulate instruction).

Fixes PR27216

[rengolin]

lib/Basic/Targets.cpp

4935

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".

test/CodeGen/arm-neon-fma.c

6

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...

test/Sema/arm_vfma.c

1

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?

[sbaranga]

lib/Basic/Targets.cpp

4935

Yes, that should be sufficient.

test/CodeGen/arm-neon-fma.c

6

Sure, good point.

test/Sema/arm_vfma.c

1

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.

[sbaranga]

Apply review comments from Renato:

• simplify condition for enabling __ARM_FEATURE_FMA
• use cortex-a7 instead of cortex-a8 for testing since this is a real use case.

[rengolin]

LGTM. Thanks!

[t.p.northover]

test/Sema/arm_vfma.c

1

v7s is Swift, which has FMA. v7 for us is Cortex-A9, which I think also has FMA (not that it matters much these days).

[rengolin]

test/Sema/arm_vfma.c

1

v7 is Cortex-A8, and neither A8 nor A9 have FMA in VFP, only NEON.

Does Swift have FMA in VFP? or just NEON?

[t.p.northover]

test/Sema/arm_vfma.c

1

Sorry, it appears virtually every part of my statement was wrong then. v7 really does seem to be Cortex-A8 even for us, and Swift doesn't have scalar VFMA.

[sbaranga]

test/Sema/arm_vfma.c

1

The error seems to be coming from how the getDefaultFPU() is called when the cpu is not specified. It turns out that it gets called with an empty CPU string (perhaps we meant to call with either "generic" or the CPU that was set in ARMTargetInfo (which does get correctly recognized as swift in this case).

FWIW, Cortex-A9 doesn't have FMA,

[sbaranga]

If no cpu has been passed to the command line, use the generic cpu when selecting
features/FPU, instead of using an empty string (which is not recognized by the
TargetParser).

[sbaranga]

I've updated the patch to fix the defaults when the cpu is not specified. Renato, Tim, could you have a look at this again please?

Thanks,
Silviu

[sbaranga]

A gentle ping?

Cheers,
Silviu

[rengolin]

lib/Basic/Targets.cpp

4710

This change is unrelated and may bring side effects into clang. I'd keep this out and investigate it in another patch with the appropriate tests. If you just force the target-feature in the test, this corner case won't be relevant in this patch.

test/Sema/arm_vfma.c

1

This test should not be relying on the front-end getting the feature right, it should be forcing the vfpv4 target feature on a base arch like "arm-none-eabi".

test/Sema/neon-vector-types-support.c

1 ↗

(On Diff #53409)

This change seems unrelated?

[sbaranga]

lib/Basic/Targets.cpp

4710

Ok, that makes sense. I'll revert to the previous revision of this patch.

[sbaranga]

Address the latest review comments (which means rolling back to the last change).

[sbaranga]

test/Sema/arm_vfma.c

1

I updated this test, but used thumbv7-none-eabi here, since VFPv4 requires at least v7.

[rengolin]

LGTM! Thanks!

[sbaranga]

Thanks, r267869!

-Silviu

lib/Basic/Targets.cpp

bool setABI(const std::string &Name) override {
     return false;
   }
 
   // FIXME: This should be based on Arch attributes, not CPU names.
   bool
   initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  StringRef CPU,
                  const std::vector<std::string> &FeaturesVec) const override {
 
     std::vector<const char*> TargetFeatures;
     unsigned Arch = llvm::ARM::parseArch(getTriple().getArchName());
 
     // get default FPU features
     unsigned FPUKind = llvm::ARM::getDefaultFPU(CPU, Arch);
     llvm::ARM::getFPUFeatures(FPUKind, TargetFeatures);
 
     // get default Extension features
void getTargetDefines(const LangOptions &Opts,
     // ACLE predefines.
     Builder.defineMacro("__ARM_ACLE", "200");
 
     // FP16 support (we currently only support IEEE format).
     Builder.defineMacro("__ARM_FP16_FORMAT_IEEE", "1");
     Builder.defineMacro("__ARM_FP16_ARGS", "1");
 
     // ACLE 6.5.3 Fused multiply-accumulate (FMA)
-    if (ArchVersion >= 7 && (CPUProfile != "M" || CPUAttr == "7EM"))
+    if (ArchVersion >= 7 && (FPU & VFP4FPU))
       Builder.defineMacro("__ARM_FEATURE_FMA", "1");
 
     // Subtarget options.
 
     // FIXME: It's more complicated than this and we don't really support
     // interworking.
     // Windows on ARM does not "support" interworking
     if (5 <= ArchVersion && ArchVersion <= 8 && !getTriple().isOSWindows())
       Builder.defineMacro("__THUMB_INTERWORK__");

test/CodeGen/arm-neon-fma.c

 // RUN: %clang_cc1 -triple thumbv7-none-linux-gnueabihf \
 // RUN:   -target-abi aapcs \
-// RUN:   -target-cpu cortex-a8 \
+// RUN:   -target-cpu cortex-a7 \
 // RUN:   -mfloat-abi hard \
 // RUN:   -ffreestanding \
 // RUN:   -emit-llvm -o - %s | opt -S -mem2reg | FileCheck %s
 
 #include <arm_neon.h>
 
 // CHECK-LABEL: define <2 x float> @test_fma_order(<2 x float> %accum, <2 x float> %lhs, <2 x float> %rhs) #0 {
 // CHECK:   [[TMP0:%.*]] = bitcast <2 x float> %accum to <8 x i8>
 // CHECK:   [[TMP1:%.*]] = bitcast <2 x float> %lhs to <8 x i8>
 // CHECK:   [[TMP2:%.*]] = bitcast <2 x float> %rhs to <8 x i8>
 // CHECK:   [[TMP3:%.*]] = bitcast <8 x i8> [[TMP0]] to <2 x float>

test/Preprocessor/arm-acle-6.5.c

 // RUN: %clang -target armv4-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
 // RUN: %clang -target armv5-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
 // RUN: %clang -target armv6-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
 // RUN: %clang -target armv6m-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
 // RUN: %clang -target armv7m-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
 
 // CHECK-NO-FMA-NOT: __ARM_FEATURE_FMA
 
-// RUN: %clang -target armv7a-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-FMA
-// RUN: %clang -target armv7r-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-FMA
+// RUN: %clang -target armv7a-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
+// RUN: %clang -target armv7a-eabi -mfpu=vfpv4 -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-FMA
+// RUN: %clang -target armv7r-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
+// RUN: %clang -target armv7r-eabi -mfpu=vfpv4 -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-FMA
 // RUN: %clang -target armv7em-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-FMA
-// RUN: %clang -target armv8-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-FMA
+// RUN: %clang -target armv8-eabi -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-FMA
+// RUN: %clang -target armv8-eabi -mfpu=vfpv4 -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-FMA
 
 // CHECK-FMA: __ARM_FEATURE_FMA 1
 
 // RUN: %clang -target armv4-eabi -mfpu=neon -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-NEON
 // RUN: %clang -target armv5-eabi -mfpu=neon -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-NEON
 // RUN: %clang -target armv6-eabi -mfpu=neon -x c -E -dM %s -o - | FileCheck %s -check-prefix CHECK-NO-NEON
 
 // CHECK-NO-NEON-NOT: __ARM_NEON

test/Sema/arm_vfma.c

-// RUN: %clang_cc1 -triple thumbv7s-apple-ios7.0 -target-feature +neon -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple thumbv7-none-eabi -target-feature +neon -target-feature +vfp4 -fsyntax-only -verify %s
 #include <arm_neon.h>
 
 // expected-no-diagnostics
 
 void func(float32x2_t v2f32, float32x4_t v4f32) {
   vfma_f32(v2f32, v2f32, v2f32);
   vfmaq_f32(v4f32, v4f32, v4f32);
 
   vfms_f32(v2f32, v2f32, v2f32);
   vfmsq_f32(v4f32, v4f32, v4f32);
 }