[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
simon_tatham marked an inline comment as done. simon_tatham added a comment. Hmm, yes, I see what you mean. It //looks// to me as if the problem is that `llvm::ARM::getFPUFeatures` is setting up a feature list including `+vfp4`, `-fp64` and `-d32` just as you'd expect, but when it's called from `clang::targets::ARMTargetInfo::initFeatureMap` in particular, that silently throws away any feature name in the list that doesn't start with `+`. I suppose that means `getFPUFeatures` ought to be more careful, and add the more restricted feature `vfp4d16sp` in the first place instead of trying to do it by adding full `vfp4` and then taking pieces away. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60691/new/ https://reviews.llvm.org/D60691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
JamesNagurne added a comment. Hi Simon et. al., I'm working on a downstream ARM toolchain and have downstreamed this change into our codebase. We saw that you've fixed the -mfpu=none issue and have taken that as well, but are still running into some issues. Prior to your change, the optionset "-mcpu=cortex-m4 -mfloat-abi=hard" created the following target features in the LLVM IR coming out of clang: "target-features"="+armv7e-m,+d16,+dsp,+fp-only-sp,+hwdiv,+thumb-mode,+vfp4,-crc,-dotprod,-fp16fml,-hwdiv-arm,-ras" In specific, note the +d16, +fp-only-sp. After your changes, we're not seeing the associated -d32, -fp64 target features: "target-cpu"="cortex-m4" "target-features"="+armv7e-m,+dsp,+hwdiv,+thumb-mode,+vfp4,-crc,-dotprod,-fp16fml,-hwdiv-arm,-ras" As a result, we are getting linktime failures between our libraries, which use "-mcpu=cortex-m4 -mfloat-abi=hard", and our tests, which specifically call out the FPU version being used. An interesting note is that the target machine table has the following: def : ProcessorModel<"cortex-m4", CortexM4Model,[ARMv7em, FeatureVFP4_D16_SP, FeaturePrefLoopAlign32, FeatureHasSlowFPVMLx, FeatureUseMISched, FeatureUseAA, FeatureHasNoBranchPredictor]>; Would this not mean that we'd expect vfp4-d16-sp when not otherwise specified? If not, then what is the expected behavior of -mfloat-abi=hard in the absence of an -mfpu specification? Thanks, J.B. Nagurne Texas Instruments Code Generation Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60691/new/ https://reviews.llvm.org/D60691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
This revision was automatically updated to reflect the committed changes.
Closed by commit rC361845: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
(authored by statham, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D60691?vs=201658=201691#toc
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60691/new/
https://reviews.llvm.org/D60691
Files:
lib/Basic/Targets/ARM.cpp
test/CodeGen/arm-target-features.c
test/Driver/arm-mfpu.c
Index: lib/Basic/Targets/ARM.cpp
===
--- lib/Basic/Targets/ARM.cpp
+++ lib/Basic/Targets/ARM.cpp
@@ -400,8 +400,7 @@
HasFloat16 = true;
// This does not diagnose illegal cases like having both
- // "+vfpv2" and "+vfpv3" or having "+neon" and "+fp-only-sp".
- uint32_t HW_FP_remove = 0;
+ // "+vfpv2" and "+vfpv3" or having "+neon" and "-fp64".
for (const auto : Features) {
if (Feature == "+soft-float") {
SoftFloat = true;
@@ -409,19 +408,19 @@
SoftFloatABI = true;
} else if (Feature == "+vfp2") {
FPU |= VFP2FPU;
- HW_FP |= HW_FP_SP | HW_FP_DP;
+ HW_FP |= HW_FP_SP;
} else if (Feature == "+vfp3") {
FPU |= VFP3FPU;
- HW_FP |= HW_FP_SP | HW_FP_DP;
+ HW_FP |= HW_FP_SP;
} else if (Feature == "+vfp4") {
FPU |= VFP4FPU;
- HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+ HW_FP |= HW_FP_SP | HW_FP_HP;
} else if (Feature == "+fp-armv8") {
FPU |= FPARMV8;
- HW_FP |= HW_FP_SP | HW_FP_DP | HW_FP_HP;
+ HW_FP |= HW_FP_SP | HW_FP_HP;
} else if (Feature == "+neon") {
FPU |= NeonFPU;
- HW_FP |= HW_FP_SP | HW_FP_DP;
+ HW_FP |= HW_FP_SP;
} else if (Feature == "+hwdiv") {
HWDiv |= HWDivThumb;
} else if (Feature == "+hwdiv-arm") {
@@ -432,8 +431,8 @@
Crypto = 1;
} else if (Feature == "+dsp") {
DSP = 1;
-} else if (Feature == "+fp-only-sp") {
- HW_FP_remove |= HW_FP_DP;
+} else if (Feature == "+fp64") {
+ HW_FP |= HW_FP_DP;
} else if (Feature == "+8msecext") {
if (CPUProfile != "M" || ArchVersion != 8) {
Diags.Report(diag::err_target_unsupported_mcmse) << CPU;
@@ -449,7 +448,6 @@
DotProd = true;
}
}
- HW_FP &= ~HW_FP_remove;
switch (ArchVersion) {
case 6:
Index: test/CodeGen/arm-target-features.c
===
--- test/CodeGen/arm-target-features.c
+++ test/CodeGen/arm-target-features.c
@@ -1,23 +1,23 @@
// REQUIRES: arm-registered-target
// RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu cortex-a8 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP3
-// CHECK-VFP3: "target-features"="+armv7-a,+dsp,+neon,+thumb-mode,+vfp3"
+// CHECK-VFP3: "target-features"="+armv7-a,+d32,+dsp,+fp64,+neon,+thumb-mode,+vfp3"
// RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu cortex-a5 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4
-// CHECK-VFP4: "target-features"="+armv7-a,+dsp,+neon,+thumb-mode,+vfp4"
+// CHECK-VFP4: "target-features"="+armv7-a,+d32,+dsp,+fp64,+neon,+thumb-mode,+vfp4"
// RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu cortex-a7 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV
// RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-a12 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV
// RUN: %clang_cc1 -triple thumbv7s-linux-gnueabi -target-cpu swift -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV-2
// RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu krait -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV
-// CHECK-VFP4-DIV: "target-features"="+armv7-a,+dsp,+hwdiv,+hwdiv-arm,+neon,+thumb-mode,+vfp4"
-// CHECK-VFP4-DIV-2: "target-features"="+armv7s,+dsp,+hwdiv,+hwdiv-arm,+neon,+thumb-mode,+vfp4"
+// CHECK-VFP4-DIV: "target-features"="+armv7-a,+d32,+dsp,+fp64,+hwdiv,+hwdiv-arm,+neon,+thumb-mode,+vfp4"
+// CHECK-VFP4-DIV-2: "target-features"="+armv7s,+d32,+dsp,+fp64,+hwdiv,+hwdiv-arm,+neon,+thumb-mode,+vfp4"
// RUN: %clang_cc1 -triple armv7-linux-gnueabihf -target-cpu cortex-a15 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV-ARM
// RUN: %clang_cc1 -triple armv7-linux-gnueabihf -target-cpu cortex-a17 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV-ARM
-// CHECK-VFP4-DIV-ARM: "target-features"="+armv7-a,+dsp,+hwdiv,+hwdiv-arm,+neon,+vfp4,-thumb-mode"
+// CHECK-VFP4-DIV-ARM: "target-features"="+armv7-a,+d32,+dsp,+fp64,+hwdiv,+hwdiv-arm,+neon,+vfp4,-thumb-mode"
// RUN: %clang_cc1 -triple thumbv7s-apple-ios7.0 -target-cpu cyclone -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
// RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu cortex-a32 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
@@ -28,34 +28,34 @@
// RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
SjoerdMeijer accepted this revision. SjoerdMeijer added a comment. This revision is now accepted and ready to land. There already was consensus that this is was a good change, and also that we don't care about auto-upgrading. With the last minor comments addressed, this looks good I think. Perhaps you can wait with committing until tomorrow morning just in case there are more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60691/new/ https://reviews.llvm.org/D60691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
simon_tatham marked 4 inline comments as done.
simon_tatham added inline comments.
Comment at: llvm/lib/Target/ARM/ARMSubtarget.h:587
bool hasVFP2() const { return HasVFPv2; }
bool hasVFP3() const { return HasVFPv3; }
ostannard wrote:
> Are the old functions still used anywhere? If they are not used (much), I
> think it would be better to just have one set of functions for the base FPU
> version, and check hasFP64 and hasD32 where needed, to avoid the rick of
> using the wrong version of these functions.
Indeed, it turned out the old functions weren't used anywhere at all.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60691/new/
https://reviews.llvm.org/D60691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
simon_tatham marked an inline comment as done. simon_tatham added inline comments. Comment at: llvm/test/MC/ARM/armv8.3a-js.s:16 // REQ-V83: error: instruction requires: armv8.3a -// REQ-FP: error: instruction requires: FPARMv8 +// REQ-FP: error: invalid instruction ostannard wrote: > Do you know why this diagnostic got worse? I do now :-) It's because that instruction is invalid for two reasons: firstly, `vjcvt` requires FP-ARMv8 which the command line has turned off, and secondly, using d18 requires the new `d32` feature which the command line has also turned off. So `llc -debug` reports "Opcode result: multiple types of mismatch, so not reporting near-miss". Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60691/new/ https://reviews.llvm.org/D60691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
ostannard added inline comments.
Comment at: clang/test/Driver/arm-mfpu.c:112
+// CHECK-VFP3XD-NOT: "-target-feature" "+fp64"
+// CHECK-VFP3XD-NOT: "-target-feature" "+32"
// CHECK-VFP3XD: "-target-feature" "+vfp3"
"+d32" ?
Comment at: llvm/lib/Target/ARM/ARMSubtarget.h:587
bool hasVFP2() const { return HasVFPv2; }
bool hasVFP3() const { return HasVFPv3; }
Are the old functions still used anywhere? If they are not used (much), I think
it would be better to just have one set of functions for the base FPU version,
and check hasFP64 and hasD32 where needed, to avoid the rick of using the wrong
version of these functions.
Comment at: llvm/test/MC/ARM/armv8.3a-js.s:16
// REQ-V83: error: instruction requires: armv8.3a
-// REQ-FP: error: instruction requires: FPARMv8
+// REQ-FP: error: invalid instruction
Do you know why this diagnostic got worse?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60691/new/
https://reviews.llvm.org/D60691
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
dmgreen added a comment. I don't think we here care about auto-updating, not supporting bitcode/lto libraries. Comment at: llvm/lib/Target/ARM/ARMFastISel.cpp:1362 + if (Ty->isDoubleTy() && (!Subtarget->hasVFP2Base() || !Subtarget->hasFP64())) + return false; Some of the formatting here got a little messed up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60691/new/ https://reviews.llvm.org/D60691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
ostannard edited reviewers, added: ostannard; removed: olista01. ostannard added a comment. For the auto-upgrader, these limited FPUs only exist for microcontrollers, where I doubt any users are keeping bitcode files around for a long time, so I'd be fine with not doing this. I've had a skim through the auto-upgrader code, and I don't see any other target-specific attributes which are handled there, though there are some target-specific intrinsics. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60691/new/ https://reviews.llvm.org/D60691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D60691: [ARM] Replace fp-only-sp and d16 with fp64 and d32.
t.p.northover added a comment. I like the direction of this change, and the details look correct too. The one thing I wonder about is whether we should be upgrading .bc files too (or otherwise support fp-only-sp in legacy inputs). I think it's a specialized enough feature that there won't be much older IR being mixed in, but can't be sure there's none. Anyone else got any views? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60691/new/ https://reviews.llvm.org/D60691 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
