SjoerdMeijer updated this revision to Diff 202934.
SjoerdMeijer added a comment.
Hi Oliver, thanks for your comments!
This was the easy one, they have been added:
> I also don't see any tests for the negated forms of either feature.
The trouble begun with this:
> +fp.dp, but the FPU is already double-precision
> +fp.dp, but no double-precision FPU exists (are there any FPUs which cause
> this?)
> +[no]fp or +[no]fp.dp for a CPU/arch which doesn't have any FPUs.
Because I found that basically none of this worked. The main reason was that we
were always passing `generic`. To address that we at least have a chance of
seeing a sensible CPU name, I have swapped the order of parsing -march and
-mcpu. I.e., we parse -mcpu first, and pass that to `checkARMArchName`, which
will eventually call `appendArchExtFeatures`. I think that makes more sense
when we use the CPUname to query `getDefaultFPU`.
Then about the more fancy diagnostics (e.g. "fp.dp, but the FPU is already
double-precision"): I've removed any attempt to throw clever diagnostics. I
don't think, in general, that we provide this kind of service level. I.e., we
need to do a lot more work here to avoid a meaningless, confusing, and thus
useless "--march=... not supported" error message when we provide +fp.dp on
the -march when e.g. the CPU already enabled this.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60697/new/
https://reviews.llvm.org/D60697
Files:
clang/lib/Driver/ToolChains/Arch/ARM.cpp
clang/lib/Driver/ToolChains/Arch/ARM.h
clang/test/Driver/armv8.1m.main.c
clang/test/Driver/armv8.1m.main.s
llvm/include/llvm/Support/ARMTargetParser.h
llvm/lib/Support/ARMTargetParser.cpp
Index: llvm/lib/Support/ARMTargetParser.cpp
===================================================================
--- llvm/lib/Support/ARMTargetParser.cpp
+++ llvm/lib/Support/ARMTargetParser.cpp
@@ -484,22 +484,85 @@
return StringRef();
}
-StringRef ARM::getArchExtFeature(StringRef ArchExt) {
- if (ArchExt.startswith("no")) {
- StringRef ArchExtBase(ArchExt.substr(2));
- for (const auto AE : ARCHExtNames) {
- if (AE.NegFeature && ArchExtBase == AE.getName())
- return StringRef(AE.NegFeature);
- }
+static bool stripNegationPrefix(StringRef &Name) {
+ if (Name.startswith("no")) {
+ Name = Name.substr(2);
+ return true;
}
+ return false;
+}
+
+StringRef ARM::getArchExtFeature(StringRef ArchExt) {
+ bool Negated = stripNegationPrefix(ArchExt);
for (const auto AE : ARCHExtNames) {
if (AE.Feature && ArchExt == AE.getName())
- return StringRef(AE.Feature);
+ return StringRef(Negated ? AE.NegFeature : AE.Feature);
}
return StringRef();
}
+static unsigned findDoublePrecisionFPU(unsigned InputFPUKind) {
+ const ARM::FPUName &InputFPU = ARM::FPUNames[InputFPUKind];
+
+ // If the input FPU already supports double-precision, then there
+ // isn't any different FPU we can return here.
+ //
+ // The current available FPURestriction values are None (no
+ // restriction), D16 (only 16 d-regs) and SP_D16 (16 d-regs
+ // and single precision only); there's no value representing
+ // SP restriction without D16. So this test just means 'is it
+ // SP only?'.
+ if (InputFPU.Restriction != ARM::FPURestriction::SP_D16)
+ return ARM::FK_INVALID;
+
+ // Otherwise, look for an FPU entry with all the same fields, except
+ // that SP_D16 has been replaced with just D16, representing adding
+ // double precision and not changing anything else.
+ for (const ARM::FPUName &CandidateFPU : ARM::FPUNames) {
+ if (CandidateFPU.FPUVer == InputFPU.FPUVer &&
+ CandidateFPU.NeonSupport == InputFPU.NeonSupport &&
+ CandidateFPU.Restriction == ARM::FPURestriction::D16) {
+ return CandidateFPU.ID;
+ }
+ }
+
+ // nothing found
+ return ARM::FK_INVALID;
+}
+
+bool ARM::appendArchExtFeatures(
+ StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+ std::vector<StringRef> &Features) {
+ StringRef StandardFeature = getArchExtFeature(ArchExt);
+ if (!StandardFeature.empty()) {
+ Features.push_back(StandardFeature);
+ return true;
+ }
+
+ const bool Negated = stripNegationPrefix(ArchExt);
+
+ if (CPU == "")
+ CPU = "generic";
+
+ if (ArchExt == "fp" || ArchExt == "fp.dp") {
+ unsigned FPUKind;
+ if (ArchExt == "fp.dp") {
+ if (Negated) {
+ Features.push_back("-fp64");
+ return true;
+ }
+ FPUKind = findDoublePrecisionFPU(getDefaultFPU(CPU, AK));
+ } else if (Negated) {
+ FPUKind = ARM::FK_NONE;
+ } else {
+ FPUKind = getDefaultFPU(CPU, AK);
+ }
+ return ARM::getFPUFeatures(FPUKind, Features);
+ }
+ return false;
+}
+
StringRef ARM::getHWDivName(unsigned HWDivKind) {
for (const auto D : HWDivNames) {
if (HWDivKind == D.ID)
Index: llvm/include/llvm/Support/ARMTargetParser.h
===================================================================
--- llvm/include/llvm/Support/ARMTargetParser.h
+++ llvm/include/llvm/Support/ARMTargetParser.h
@@ -240,6 +240,8 @@
StringRef getSubArch(ArchKind AK);
StringRef getArchExtName(unsigned ArchExtKind);
StringRef getArchExtFeature(StringRef ArchExt);
+bool appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+ std::vector<StringRef> &Features);
StringRef getHWDivName(unsigned HWDivKind);
// Information by Name
Index: clang/test/Driver/armv8.1m.main.s
===================================================================
--- clang/test/Driver/armv8.1m.main.s
+++ clang/test/Driver/armv8.1m.main.s
@@ -5,10 +5,24 @@
# RUN: FileCheck --check-prefix=ERROR-V81M < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+dsp -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_DSP < %t %s
+# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+fp -o /dev/null %s 2>%t
+# RUN: FileCheck --check-prefix=ERROR-V81M_FP < %t %s
+# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nofp -o /dev/null %s 2>%t
+# RUN: FileCheck --check-prefix=ERROR-V81M_FP < %t %s
+# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+fp.dp -o /dev/null %s 2>%t
+# RUN: FileCheck --check-prefix=ERROR-V81M_FPDP < %t %s
+# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nofp.dp -o /dev/null %s 2>%t
+# RUN: FileCheck --check-prefix=ERROR-V81M_FPDP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+mve -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_MVE < %t %s
+# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nomve -o /dev/null %s 2>%t
+# RUN: FileCheck --check-prefix=ERROR-V81M_MVE < %t %s
+# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+mve+fp -o /dev/null %s 2>%t
+# RUN: FileCheck --check-prefix=ERROR-V81M_MVE_FP < %t %s
# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+mve.fp -o /dev/null %s 2>%t
# RUN: FileCheck --check-prefix=ERROR-V81M_MVEFP < %t %s
+# RUN: not %clang -c -target arm-none-none-eabi -march=armv8.1-m.main+nomve.fp -o /dev/null %s 2>%t
+# RUN: FileCheck --check-prefix=ERROR-V81M_MVEFP < %t %s
.syntax unified
.thumb
@@ -39,15 +53,21 @@
# ERROR-V8M: :[[@LINE-1]]:1: error
# ERROR-V81M: :[[@LINE-2]]:1: error
# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
-# ERROR-V81M_MVE: :[[@LINE-4]]:1: error
-# ERROR-V81M_MVEFP: :[[@LINE-5]]:1: error
+# ERROR-V81M_FP: :[[@LINE-4]]:1: error
+# ERROR-V81M_MVE: :[[@LINE-5]]:1: error
+# ERROR-V81M_MVE_FP: :[[@LINE-6]]:1: error
+# ERROR-V81M_MVEFP: :[[@LINE-7]]:1: error
asrl r0, r1, r2
# ERROR-V8M: :[[@LINE-1]]:1: error
# ERROR-V81M: :[[@LINE-2]]:1: error
# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
+# ERROR-V81M_FP: :[[@LINE-4]]:1: error
+# ERROR-V81M_FPDP: :[[@LINE-5]]:1: error
vcadd.i8 q0, q1, q2, #90
# ERROR-V8M: :[[@LINE-1]]:1: error
# ERROR-V81M: :[[@LINE-2]]:1: error
# ERROR-V81M_DSP: :[[@LINE-3]]:1: error
+# ERROR-V81M_FP: :[[@LINE-4]]:1: error
+# ERROR-V81M_FPDP: :[[@LINE-5]]:1: error
Index: clang/test/Driver/armv8.1m.main.c
===================================================================
--- clang/test/Driver/armv8.1m.main.c
+++ clang/test/Driver/armv8.1m.main.c
@@ -2,13 +2,48 @@
// RUN: FileCheck --check-prefix=CHECK-DSP < %t %s
// CHECK-DSP: "-target-feature" "+dsp"
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+fp -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-FP < %t %s
+// CHECK-FP: "-target-feature" "+fp-armv8"
+// CHECK-FP-NOT: "-target-feature" "+fp64"
+// CHECK-FP-NOT: "-target-feature" "+d32"
+// CHECK-FP: "-target-feature" "+fullfp16"
+
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+nofp -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-NOFP < %t %s
+// CHECK-NOFP: "-target-feature" "-vfp2" "-target-feature" "-vfp3" "-target-feature" "-fp16" "-target-feature" "-vfp4" "-target-feature" "-fp-armv8" "-target-feature" "-fp64" "-target-feature" "-d32" "-target-feature" "-neon" "-target-feature" "-crypto"
+
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+fp.dp -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-FPDP < %t %s
+// CHECK-FPDP: "-target-feature" "+fp-armv8"
+// CHECK-FPDP: "-target-feature" "+fullfp16"
+// CHECK-FPDP: "-target-feature" "+fp64"
+// CHECK-FPDP-NOT: "-target-feature" "+d32"
+
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+nofp.dp -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-NOFPDP < %t %s
+// CHECK-NOFPDP: "-target-feature" "-fp64"
+
// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+mve -### %s 2> %t
// RUN: FileCheck --check-prefix=CHECK-MVE < %t %s
// CHECK-MVE: "-target-feature" "+mve"
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+nomve -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-NOMVE < %t %s
+// CHECK-NOMVE: "-target-feature" "-mve"
+
// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+mve.fp -### %s 2> %t
// RUN: FileCheck --check-prefix=CHECK-MVEFP < %t %s
// CHECK-MVEFP: "-target-feature" "+mve.fp"
// CHECK-MVEFP-NOT: "-target-feature" "+fp64"
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+nomve.fp -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-NOMVEFP < %t %s
+// CHECK-NOMVEFP: "-target-feature" "-mve.fp"
+
+// RUN: %clang -target arm-arm-none-eabi -march=armv8.1-m.main+mve.fp+fp.dp -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-MVEFP_DP < %t %s
+// CHECK-MVEFP_DP: "-target-feature" "+mve.fp"
+// CHECK-MVEFP_DP: "-target-feature" "+fp64"
+
double foo (double a) { return a; }
Index: clang/lib/Driver/ToolChains/Arch/ARM.h
===================================================================
--- clang/lib/Driver/ToolChains/Arch/ARM.h
+++ clang/lib/Driver/ToolChains/Arch/ARM.h
@@ -13,6 +13,7 @@
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/Triple.h"
#include "llvm/Option/Option.h"
+#include "llvm/Support/TargetParser.h"
#include <string>
#include <vector>
@@ -25,6 +26,8 @@
const llvm::Triple &Triple);
const std::string getARMArch(llvm::StringRef Arch, const llvm::Triple &Triple);
StringRef getARMCPUForMArch(llvm::StringRef Arch, const llvm::Triple &Triple);
+llvm::ARM::ArchKind getLLVMArchKindForARM(StringRef CPU, StringRef Arch,
+ const llvm::Triple &Triple);
StringRef getLLVMArchSuffixForARM(llvm::StringRef CPU, llvm::StringRef Arch,
const llvm::Triple &Triple);
Index: clang/lib/Driver/ToolChains/Arch/ARM.cpp
===================================================================
--- clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -72,15 +72,13 @@
// Decode ARM features from string like +[no]featureA+[no]featureB+...
static bool DecodeARMFeatures(const Driver &D, StringRef text,
+ StringRef CPU, llvm::ARM::ArchKind ArchKind,
std::vector<StringRef> &Features) {
SmallVector<StringRef, 8> Split;
text.split(Split, StringRef("+"), -1, false);
for (StringRef Feature : Split) {
- StringRef FeatureName = llvm::ARM::getArchExtFeature(Feature);
- if (!FeatureName.empty())
- Features.push_back(FeatureName);
- else
+ if (!appendArchExtFeatures(CPU, ArchKind, Feature, Features))
return false;
}
return true;
@@ -100,14 +98,16 @@
// getARMArch is used here instead of just checking the -march value in order
// to handle -march=native correctly.
static void checkARMArchName(const Driver &D, const Arg *A, const ArgList &Args,
- llvm::StringRef ArchName,
+ llvm::StringRef ArchName, llvm::StringRef CPUName,
std::vector<StringRef> &Features,
const llvm::Triple &Triple) {
std::pair<StringRef, StringRef> Split = ArchName.split("+");
std::string MArch = arm::getARMArch(ArchName, Triple);
- if (llvm::ARM::parseArch(MArch) == llvm::ARM::ArchKind::INVALID ||
- (Split.second.size() && !DecodeARMFeatures(D, Split.second, Features)))
+ llvm::ARM::ArchKind ArchKind = llvm::ARM::parseArch(MArch);
+ if (ArchKind == llvm::ARM::ArchKind::INVALID ||
+ (Split.second.size() && !DecodeARMFeatures(
+ D, Split.second, CPUName, ArchKind, Features)))
D.Diag(clang::diag::err_drv_clang_unsupported) << A->getAsString(Args);
}
@@ -119,8 +119,11 @@
std::pair<StringRef, StringRef> Split = CPUName.split("+");
std::string CPU = arm::getARMTargetCPU(CPUName, ArchName, Triple);
- if (arm::getLLVMArchSuffixForARM(CPU, ArchName, Triple).empty() ||
- (Split.second.size() && !DecodeARMFeatures(D, Split.second, Features)))
+ llvm::ARM::ArchKind ArchKind =
+ arm::getLLVMArchKindForARM(CPU, ArchName, Triple);
+ if (ArchKind == llvm::ARM::ArchKind::INVALID ||
+ (Split.second.size() && !DecodeARMFeatures(
+ D, Split.second, CPU, ArchKind, Features)))
D.Diag(clang::diag::err_drv_clang_unsupported) << A->getAsString(Args);
}
@@ -327,25 +330,12 @@
if (ThreadPointer == arm::ReadTPMode::Cp15)
Features.push_back("+read-tp-hard");
- // Check -march. ClangAs gives preference to -Wa,-march=.
const Arg *ArchArg = Args.getLastArg(options::OPT_march_EQ);
+ const Arg *CPUArg = Args.getLastArg(options::OPT_mcpu_EQ);
StringRef ArchName;
- if (WaArch) {
- if (ArchArg)
- D.Diag(clang::diag::warn_drv_unused_argument)
- << ArchArg->getAsString(Args);
- ArchName = StringRef(WaArch->getValue()).substr(7);
- checkARMArchName(D, WaArch, Args, ArchName, Features, Triple);
- // FIXME: Set Arch.
- D.Diag(clang::diag::warn_drv_unused_argument) << WaArch->getAsString(Args);
- } else if (ArchArg) {
- ArchName = ArchArg->getValue();
- checkARMArchName(D, ArchArg, Args, ArchName, Features, Triple);
- }
+ StringRef CPUName;
// Check -mcpu. ClangAs gives preference to -Wa,-mcpu=.
- const Arg *CPUArg = Args.getLastArg(options::OPT_mcpu_EQ);
- StringRef CPUName;
if (WaCPU) {
if (CPUArg)
D.Diag(clang::diag::warn_drv_unused_argument)
@@ -355,6 +345,20 @@
} else if (CPUArg)
CPUName = CPUArg->getValue();
+ // Check -march. ClangAs gives preference to -Wa,-march=.
+ if (WaArch) {
+ if (ArchArg)
+ D.Diag(clang::diag::warn_drv_unused_argument)
+ << ArchArg->getAsString(Args);
+ ArchName = StringRef(WaArch->getValue()).substr(7);
+ checkARMArchName(D, WaArch, Args, ArchName, CPUName, Features, Triple);
+ // FIXME: Set Arch.
+ D.Diag(clang::diag::warn_drv_unused_argument) << WaArch->getAsString(Args);
+ } else if (ArchArg) {
+ ArchName = ArchArg->getValue();
+ checkARMArchName(D, ArchArg, Args, ArchName, CPUName, Features, Triple);
+ }
+
// Add CPU features for generic CPUs
if (CPUName == "native") {
llvm::StringMap<bool> HostFeatures;
@@ -620,11 +624,12 @@
return getARMCPUForMArch(Arch, Triple);
}
-/// getLLVMArchSuffixForARM - Get the LLVM arch name to use for a particular
-/// CPU (or Arch, if CPU is generic).
-// FIXME: This is redundant with -mcpu, why does LLVM use this.
-StringRef arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch,
- const llvm::Triple &Triple) {
+/// getLLVMArchSuffixForARM - Get the LLVM ArchKind value to use for a
+/// particular CPU (or Arch, if CPU is generic). This is needed to
+/// pass to functions like llvm::ARM::getDefaultFPU which need an
+/// ArchKind as well as a CPU name.
+llvm::ARM::ArchKind arm::getLLVMArchKindForARM(StringRef CPU, StringRef Arch,
+ const llvm::Triple &Triple) {
llvm::ARM::ArchKind ArchKind;
if (CPU == "generic") {
std::string ARMArch = tools::arm::getARMArch(Arch, Triple);
@@ -640,6 +645,15 @@
? llvm::ARM::ArchKind::ARMV7K
: llvm::ARM::parseCPUArch(CPU);
}
+ return ArchKind;
+}
+
+/// getLLVMArchSuffixForARM - Get the LLVM arch name to use for a particular
+/// CPU (or Arch, if CPU is generic).
+// FIXME: This is redundant with -mcpu, why does LLVM use this.
+StringRef arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch,
+ const llvm::Triple &Triple) {
+ llvm::ARM::ArchKind ArchKind = getLLVMArchKindForARM(CPU, Arch, Triple);
if (ArchKind == llvm::ARM::ArchKind::INVALID)
return "";
return llvm::ARM::getSubArch(ArchKind);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
