Make march/target-cpu print a note with the list of valid values for ARM

[erichkeane]

Summary

When rejecting a march= or target-cpu command line parameter,
the message is quite lacking. This patch adds a note that prints
all possible values for the current target, if the target supports it.

This adds support for the ARM/AArch64 targets (more to come!). These
two also require: https://reviews.llvm.org/D42979

[erichkeane]

LLVM patch here: https://reviews.llvm.org/D42979

[samparker]

No tests?

[fhahn]

I like the idea. However for all backends, except Arm and AArch64, we would have to maintain another list of CPU names. At least for the targets which implement isValidCPUName, we could add an array with valid names and use that. That's still not ideal, but I do not think we should hold back this patch until all targets use the same infrastructure to handle CPUs. As for tests, we should test the note for all backends I think.

test/Misc/target-parser.c

3 ↗

(On Diff #133049)

drop one set of //

[fhahn]

Also with tests for each backend, this diff will get quite big. As this is opt-in, it might make sense to enable backends separately.

[Hahnfeld]

I think this means that the Clang test needs to be updated whenever somebody adds an architecture to LLVM? Maybe just test that Clang emits a note and don't check which values it prints? These should be checked in the backend...

[fhahn]

In D42978#1001616, @Hahnfeld wrote:

I think this means that the Clang test needs to be updated whenever somebody adds an architecture to LLVM? Maybe just test that Clang emits a note and don't check which values it prints? These should be checked in the backend...

Yes, I agree checking all CPUs names is too fragile and not necessary for AArch64 or ARM, as the list of CPUs is provided by llvm.

It is probably enough to check if we emit the note for AArch64 and ARM and maybe check that it is non-empty or checking that it contains a CPU we know should always be there. Checking the full list may make sense for other backends, as we have to modify Clang directly to add support.

[erichkeane]

That makes sense. I changed the ARM one to check ONLY the first one, since in each case that seems like the oldest anyway. It at least makes sure that the lists are distinct, and avoids this cross-repo fragility.

[erichkeane]

Simplified ARM tests as requested.

[fhahn]

LGTM thanks. Please wait a day or so with committing so others can raise additional concerns.

test/Misc/target-invalid-cpu-note.c

8

stray newline? :)

[erichkeane]

Removed extra newline.

[Hahnfeld]

test/Misc/target-invalid-cpu-note.c

2

Is there a reason you don't use -verify=<prefix> in this test? That's what I've usually seen for checking errors and notes...

4

Is this guaranteed to be first? If not, you might want to add {{.*}} to account for future updates.

(If not using -verify as suggested above, you could also use ARM-SAME on a new-line. This should also allow arbitrary values in between.)

[erichkeane]

test/Misc/target-invalid-cpu-note.c

2

Um... wow... TIL.

4

Not guaranteed. I'll do this, thanks!

[erichkeane]

test/Misc/target-invalid-cpu-note.c

2

Sadly, -verify= doesn't seem to have a way to specify things that happen without a line-number, so I wasn't able to do it :/

[erichkeane]

Relaxed the test further.

[simpal01]

@erichkeane Could you please tell me why this was done only for -cc1 and if there is any plan to add this to the driver in general ?

include/clang/Basic/DiagnosticCommonKinds.td

 def note_invalid_subexpr_in_const_expr : Note<
   "subexpression not valid in a constant expression">;
 
 // Targets
 
 def err_target_unknown_triple : Error<
   "unknown target triple '%0', please use -triple or -arch">;
 def err_target_unknown_cpu : Error<"unknown target CPU '%0'">;
+def note_valid_options : Note<"valid target CPU values are: %0">;
 def err_target_unsupported_cpu_for_micromips : Error<
   "micromips is not supported for target CPU '%0'">;
 def err_target_unknown_abi : Error<"unknown target ABI '%0'">;
 def err_target_unsupported_abi : Error<"ABI '%0' is not supported on CPU '%1'">;
 def err_target_unsupported_abi_for_triple : Error<
   "ABI '%0' is not supported for '%1'">;
 def err_target_unknown_fpmath : Error<"unknown FP unit '%0'">;
 def err_target_unsupported_fpmath : Error<

include/clang/Basic/TargetInfo.h

public:
 
   /// \brief Target the specified CPU.
   ///
   /// \return  False on error (invalid CPU name).
   virtual bool setCPU(const std::string &Name) {
     return false;
   }
 
+  /// Fill a SmallVectorImpl with the valid values to setCPU.
+  virtual void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const {}
+
   /// brief Determine whether this TargetInfo supports the given CPU name.
   virtual bool isValidCPUName(StringRef Name) const {
     return true;
   }
 
   /// \brief Use the specified ABI.
   ///
   /// \return False on error (invalid ABI name).

lib/Basic/Targets.cpp

 #include "Targets/SPIR.h"
 #include "Targets/Sparc.h"
 #include "Targets/SystemZ.h"
 #include "Targets/TCE.h"
 #include "Targets/WebAssembly.h"
 #include "Targets/X86.h"
 #include "Targets/XCore.h"
 #include "clang/Basic/Diagnostic.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Triple.h"
 
 using namespace clang;
 
 namespace clang {
 namespace targets {
 //===----------------------------------------------------------------------===//
 //  Common code shared among targets.
if (!Target) {
     Diags.Report(diag::err_target_unknown_triple) << Triple.str();
     return nullptr;
   }
   Target->TargetOpts = Opts;
 
   // Set the target CPU if specified.
   if (!Opts->CPU.empty() && !Target->setCPU(Opts->CPU)) {
     Diags.Report(diag::err_target_unknown_cpu) << Opts->CPU;
+    SmallVector<StringRef, 32> ValidList;
+    Target->fillValidCPUList(ValidList);
+    if (!ValidList.empty())
+      Diags.Report(diag::note_valid_options) << llvm::join(ValidList, ", ");
     return nullptr;
   }
 
   // Set the target ABI if specified.
   if (!Opts->ABI.empty() && !Target->setABI(Opts->ABI)) {
     Diags.Report(diag::err_target_unknown_abi) << Opts->ABI;
     return nullptr;
   }

lib/Basic/Targets/AArch64.h

 
 public:
   AArch64TargetInfo(const llvm::Triple &Triple, const TargetOptions &Opts);
 
   StringRef getABI() const override;
   bool setABI(const std::string &Name) override;
 
   bool isValidCPUName(StringRef Name) const override;
+  void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
   bool setCPU(const std::string &Name) override;
 
   bool useFP16ConversionIntrinsics() const override {
     return false;
   }
 
   void getTargetDefinesARMV81A(const LangOptions &Opts,
                                MacroBuilder &Builder) const;

lib/Basic/Targets/AArch64.cpp

bool AArch64TargetInfo::isValidCPUName(StringRef Name) const {
   return Name == "generic" ||
          llvm::AArch64::parseCPUArch(Name) != llvm::AArch64::ArchKind::INVALID;
 }
 
 bool AArch64TargetInfo::setCPU(const std::string &Name) {
   return isValidCPUName(Name);
 }
 
+void AArch64TargetInfo::fillValidCPUList(
+    SmallVectorImpl<StringRef> &Values) const {
+  llvm::AArch64::fillValidCPUArchList(Values);
+}
+
 void AArch64TargetInfo::getTargetDefinesARMV81A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   Builder.defineMacro("__ARM_FEATURE_QRDMX", "1");
 }
 
 void AArch64TargetInfo::getTargetDefinesARMV82A(const LangOptions &Opts,
                                                 MacroBuilder &Builder) const {
   // Also include the ARMv8.1 defines

lib/Basic/Targets/ARM.h

initFeatureMap(llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags,
                  const std::vector<std::string> &FeaturesVec) const override;
 
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override;
 
   bool hasFeature(StringRef Feature) const override;
 
   bool isValidCPUName(StringRef Name) const override;
+  void fillValidCPUList(SmallVectorImpl<StringRef> &Values) const override;
+
   bool setCPU(const std::string &Name) override;
 
   bool setFPMath(StringRef Name) override;
 
   bool useFP16ConversionIntrinsics() const override {
     return false;
   }
 

lib/Basic/Targets/ARM.cpp

return llvm::StringSwitch<bool>(Feature)
       .Default(false);
 }
 
 bool ARMTargetInfo::isValidCPUName(StringRef Name) const {
   return Name == "generic" ||
          llvm::ARM::parseCPUArch(Name) != llvm::ARM::ArchKind::INVALID;
 }
 
+void ARMTargetInfo::fillValidCPUList(SmallVectorImpl<StringRef> &Values) const {
+  llvm::ARM::fillValidCPUArchList(Values);
+}
+
 bool ARMTargetInfo::setCPU(const std::string &Name) {
   if (Name != "generic")
     setArchInfo(llvm::ARM::parseCPUArch(Name));
 
   if (ArchKind == llvm::ARM::ArchKind::INVALID)
     return false;
   setAtomic();
   CPU = Name;

test/Misc/target-invalid-cpu-note.c

+// RUN: not %clang_cc1 -triple armv5--- -target-cpu not-a-cpu -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix ARM
+// ARM: error: unknown target CPU 'not-a-cpu'
+// ARM: note: valid target CPU values are: arm2,
+
+// RUN: not %clang_cc1 -triple arm64--- -target-cpu not-a-cpu -fsyntax-only %s 2>&1 | FileCheck %s --check-prefix AARCH64
+// AARCH64: error: unknown target CPU 'not-a-cpu'
+// AARCH64: note: valid target CPU values are: cortex-a35,
+