[clang][mips] Set __mips_fpr correctly for -mfpxx

[smaksimovic]

Summary

Set __mips_fpr to 0 if o32 ABI is used with either -mfpxx or none of -mfp32, -mfpxx, -mfp64 being specified.

Introduce additional checks:

• -mfpxx is only to be used in conjunction with the o32 ABI.
• report an error when incompatible options are provided.

Formerly no errors were raised when combining n32/n64 ABIs with -mfp32 and -mfpxx.

There are other cases when __mips_fpr should be set to 0 that are not covered, ex. using o32 on a mips64 cpu which is valid but not supported in the backend as of yet.

[atanasyan]

Any test cases?

lib/Basic/Targets/Mips.cpp

62

The CPU argument looks unused. We can either remove it or make this routine a non-member static function. Probably we can change the CPU’s type to StringRef.

97–98

Is this change required for the fix? It looks like a refactoring and maybe done by a separate commit.

lib/Basic/Targets/Mips.h

61

Maybe replace these two boolean flags by a single enumeration. Something like FPXX, FP32, FP64.

[smaksimovic]

Added test cases.

[smaksimovic]

lib/Basic/Targets/Mips.cpp

62

I opted to remove the argument.

97–98

Since I took the code that originally was here in order to reuse it down below, I changed this line because the new function returns an int instead, as it was easier to do comparisons at line 264.

[atanasyan]

lib/Basic/Targets/Mips.cpp

153

What do you think about this variant of the code? It's longer, but imho looks more clear.

switch (FPMode) {
case FPXX:
  Builder.defineMacro("__mips_fpr", Twine(0));
  break;
case FP32:
  Builder.defineMacro("__mips_fpr", Twine(32));
  break;
case FP64:
  Builder.defineMacro("__mips_fpr", Twine(64));
  break;
}

if (FPMode == FP64 || IsSingleFloat)
  Builder.defineMacro("_MIPS_FPSET", Twine(32));
else
  Builder.defineMacro("_MIPS_FPSET", Twine(16));

[smaksimovic]

Expanded a piece of code using switch cases as suggested.

[atanasyan]

LGTM

include/clang/Basic/DiagnosticCommonKinds.td

 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_unsupported_abi_for_opt : Error<"'%0' can only be used with the '%1' ABI">;
+def err_mips_fp64_req : Error<
+    "'%0' can only be used if the target supports the mfhc1 and mthc1 instructions">;
 def err_target_unknown_fpmath : Error<"unknown FP unit '%0'">;
 def err_target_unsupported_fpmath : Error<
     "the '%0' unit is not supported with this instruction set">;
 def err_target_unsupported_unaligned : Error<
   "the %0 sub-architecture does not support unaligned accesses">;
 def err_target_unsupported_execute_only : Error<
   "execute only is not supported for the %0 sub-architecture">;
 def err_opt_not_valid_with_opt : Error<

lib/Basic/Targets/Mips.h

class LLVM_LIBRARY_VISIBILITY MipsTargetInfo : public TargetInfo {
   bool CanUseBSDABICalls;
   enum MipsFloatABI { HardFloat, SoftFloat } FloatABI;
   enum DspRevEnum { NoDSP, DSP1, DSP2 } DspRev;
   bool HasMSA;
   bool DisableMadd4;
   bool UseIndirectJumpHazard;
 
 protected:
-  bool HasFP64;
+  enum FPModeEnum { FPXX, FP32, FP64 } FPMode;
   std::string ABI;
 
 public:
   MipsTargetInfo(const llvm::Triple &Triple, const TargetOptions &)
       : TargetInfo(Triple), IsMips16(false), IsMicromips(false),
         IsNan2008(false), IsAbs2008(false), IsSingleFloat(false),
         IsNoABICalls(false), CanUseBSDABICalls(false), FloatABI(HardFloat),
         DspRev(NoDSP), HasMSA(false), DisableMadd4(false),
-        UseIndirectJumpHazard(false), HasFP64(false) {
+        UseIndirectJumpHazard(false), FPMode(FPXX) {
     TheCXXABI.set(TargetCXXABI::GenericMIPS);
 
     setABI(getTriple().isMIPS32() ? "o32" : "n64");
 
     CPU = ABI == "o32" ? "mips32r2" : "mips64r2";
 
     CanUseBSDABICalls = Triple.getOS() == llvm::Triple::FreeBSD ||
                         Triple.getOS() == llvm::Triple::OpenBSD;
if (CPU.empty())
       CPU = getCPU();
     if (CPU == "octeon")
       Features["mips64r2"] = Features["cnmips"] = true;
     else
       Features[CPU] = true;
     return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
   }
 
+  unsigned getISARev() const;
+
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override;
 
   ArrayRef<Builtin::Info> getTargetBuiltins() const override;
 
   bool hasFeature(StringRef Feature) const override;
 
   BuiltinVaListKind getBuiltinVaListKind() const override {
bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override {
     IsMips16 = false;
     IsMicromips = false;
     IsNan2008 = isIEEE754_2008Default();
     IsAbs2008 = isIEEE754_2008Default();
     IsSingleFloat = false;
     FloatABI = HardFloat;
     DspRev = NoDSP;
-    HasFP64 = isFP64Default();
+    FPMode = isFP64Default() ? FP64 : FPXX;
 
     for (const auto &Feature : Features) {
       if (Feature == "+single-float")
         IsSingleFloat = true;
       else if (Feature == "+soft-float")
         FloatABI = SoftFloat;
       else if (Feature == "+mips16")
         IsMips16 = true;
       else if (Feature == "+micromips")
         IsMicromips = true;
       else if (Feature == "+dsp")
         DspRev = std::max(DspRev, DSP1);
       else if (Feature == "+dspr2")
         DspRev = std::max(DspRev, DSP2);
       else if (Feature == "+msa")
         HasMSA = true;
       else if (Feature == "+nomadd4")
         DisableMadd4 = true;
       else if (Feature == "+fp64")
-        HasFP64 = true;
+        FPMode = FP64;
       else if (Feature == "-fp64")
-        HasFP64 = false;
+        FPMode = FP32;
+      else if (Feature == "+fpxx")
+        FPMode = FPXX;
       else if (Feature == "+nan2008")
         IsNan2008 = true;
       else if (Feature == "-nan2008")
         IsNan2008 = false;
       else if (Feature == "+abs2008")
         IsAbs2008 = true;
       else if (Feature == "-abs2008")
         IsAbs2008 = false;

lib/Basic/Targets/Mips.cpp

bool MipsTargetInfo::isValidCPUName(StringRef Name) const {
   return llvm::find(ValidCPUNames, Name) != std::end(ValidCPUNames);
 }
 
 void MipsTargetInfo::fillValidCPUList(
     SmallVectorImpl<StringRef> &Values) const {
   Values.append(std::begin(ValidCPUNames), std::end(ValidCPUNames));
 }
 
+unsigned MipsTargetInfo::getISARev() const {
+  return llvm::StringSwitch<unsigned>(getCPU())
+             .Cases("mips32", "mips64", 1)
+             .Cases("mips32r2", "mips64r2", 2)
+             .Cases("mips32r3", "mips64r3", 3)
+             .Cases("mips32r5", "mips64r5", 5)
+             .Cases("mips32r6", "mips64r6", 6)
+             .Default(0);
+}
+
 void MipsTargetInfo::getTargetDefines(const LangOptions &Opts,
                                       MacroBuilder &Builder) const {
   if (BigEndian) {
     DefineStd(Builder, "MIPSEB", Opts);
     Builder.defineMacro("_MIPSEB");
   } else {
     DefineStd(Builder, "MIPSEL", Opts);
     Builder.defineMacro("_MIPSEL");
if (ABI == "o32") {
     Builder.defineMacro("_MIPS_ISA", "_MIPS_ISA_MIPS32");
   } else {
     Builder.defineMacro("__mips", "64");
     Builder.defineMacro("__mips64");
     Builder.defineMacro("__mips64__");
     Builder.defineMacro("_MIPS_ISA", "_MIPS_ISA_MIPS64");
   }
 
-  const std::string ISARev = llvm::StringSwitch<std::string>(getCPU())
-                                 .Cases("mips32", "mips64", "1")
-                                 .Cases("mips32r2", "mips64r2", "2")
-                                 .Cases("mips32r3", "mips64r3", "3")
-                                 .Cases("mips32r5", "mips64r5", "5")
-                                 .Cases("mips32r6", "mips64r6", "6")
-                                 .Default("");
+  const std::string ISARev = std::to_string(getISARev());
+
   if (!ISARev.empty())
     Builder.defineMacro("__mips_isa_rev", ISARev);
 
   if (ABI == "o32") {
     Builder.defineMacro("__mips_o32");
     Builder.defineMacro("_ABIO32", "1");
     Builder.defineMacro("_MIPS_SIM", "_ABIO32");
   } else if (ABI == "n32") {
void MipsTargetInfo::getTargetDefines(const LangOptions &Opts,
   case SoftFloat:
     Builder.defineMacro("__mips_soft_float", Twine(1));
     break;
   }
 
   if (IsSingleFloat)
     Builder.defineMacro("__mips_single_float", Twine(1));
 
-  Builder.defineMacro("__mips_fpr", HasFP64 ? Twine(64) : Twine(32));
-  Builder.defineMacro("_MIPS_FPSET",
-                      Twine(32 / (HasFP64 || IsSingleFloat ? 1 : 2)));
+  switch (FPMode) {
+  case FPXX:
+    Builder.defineMacro("__mips_fpr", Twine(0));
+    break;
+  case FP32:
+    Builder.defineMacro("__mips_fpr", Twine(32));
+    break;
+  case FP64:
+    Builder.defineMacro("__mips_fpr", Twine(64));
+    break;
+}
+
+  if (FPMode == FP64 || IsSingleFloat)
+    Builder.defineMacro("_MIPS_FPSET", Twine(32));
+  else
+    Builder.defineMacro("_MIPS_FPSET", Twine(16));
 
   if (IsMips16)
     Builder.defineMacro("__mips16", Twine(1));
 
   if (IsMicromips)
     Builder.defineMacro("__mips_micromips", Twine(1));
 
   if (IsNan2008)
     Builder.defineMacro("__mips_nan2008", Twine(1));
void MipsTargetInfo::getTargetDefines(const LangOptions &Opts,
   // require 64-bit GPRs and O32 only supports 32-bit GPRs.
   if (ABI == "n32" || ABI == "n64")
     Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
 }
 
 bool MipsTargetInfo::hasFeature(StringRef Feature) const {
   return llvm::StringSwitch<bool>(Feature)
       .Case("mips", true)
-      .Case("fp64", HasFP64)
+      .Case("fp64", FPMode == FP64)
       .Default(false);
 }
 
 ArrayRef<Builtin::Info> MipsTargetInfo::getTargetBuiltins() const {
   return llvm::makeArrayRef(BuiltinInfo, clang::Mips::LastTSBuiltin -
                                              Builtin::FirstTSBuiltin);
 }
 
bool MipsTargetInfo::validateTarget(DiagnosticsEngine &Diags) const {
   //        can't handle this yet. It's better to fail here than on the
   //        backend assertion.
   if (getTriple().isMIPS32() && (ABI == "n32" || ABI == "n64")) {
     Diags.Report(diag::err_target_unsupported_abi_for_triple)
         << ABI << getTriple().str();
     return false;
   }
 
+  // -fpxx is valid only for the o32 ABI
+  if (FPMode == FPXX && (ABI == "n32" || ABI == "n64")) {
+    Diags.Report(diag::err_unsupported_abi_for_opt) << "-mfpxx" << "o32";
+    return false;
+  }
+
+  // -mfp32 and n32/n64 ABIs are incompatible
+  if (FPMode != FP64 && FPMode != FPXX && !IsSingleFloat &&
+      (ABI == "n32" || ABI == "n64")) {
+    Diags.Report(diag::err_opt_not_valid_with_opt) << "-mfpxx" << CPU;
+    return false;
+  }
+  // Mips revision 6 and -mfp32 are incompatible
+  if (FPMode != FP64 && FPMode != FPXX && (CPU == "mips32r6" ||
+      CPU == "mips64r6")) {
+    Diags.Report(diag::err_opt_not_valid_with_opt) << "-mfp32" << CPU;
+    return false;
+  }
+  // Option -mfp64 permitted on Mips32 iff revision 2 or higher is present
+  if (FPMode == FP64 && (CPU == "mips1" || CPU == "mips2" ||
+      getISARev() < 2) && ABI == "o32") {
+    Diags.Report(diag::err_mips_fp64_req) << "-mfp64";
+    return false;
+  }
+
   return true;
 }

test/Preprocessor/init.c

 // MIPS32BE:#define __WCHAR_WIDTH__ 32
 // MIPS32BE:#define __WINT_TYPE__ int
 // MIPS32BE:#define __WINT_WIDTH__ 32
 // MIPS32BE:#define __clang__ 1
 // MIPS32BE:#define __llvm__ 1
 // MIPS32BE:#define __mips 32
 // MIPS32BE:#define __mips__ 1
 // MIPS32BE:#define __mips_abicalls 1
-// MIPS32BE:#define __mips_fpr 32
+// MIPS32BE:#define __mips_fpr 0
 // MIPS32BE:#define __mips_hard_float 1
 // MIPS32BE:#define __mips_o32 1
 // MIPS32BE:#define _mips 1
 // MIPS32BE:#define mips 1
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=mipsel-none-none < /dev/null | FileCheck -match-full-lines -check-prefix MIPS32EL %s
 //
 // MIPS32EL:#define MIPSEL 1

 // MIPS32EL:#define __WCHAR_WIDTH__ 32
 // MIPS32EL:#define __WINT_TYPE__ int
 // MIPS32EL:#define __WINT_WIDTH__ 32
 // MIPS32EL:#define __clang__ 1
 // MIPS32EL:#define __llvm__ 1
 // MIPS32EL:#define __mips 32
 // MIPS32EL:#define __mips__ 1
 // MIPS32EL:#define __mips_abicalls 1
-// MIPS32EL:#define __mips_fpr 32
+// MIPS32EL:#define __mips_fpr 0
 // MIPS32EL:#define __mips_hard_float 1
 // MIPS32EL:#define __mips_o32 1
 // MIPS32EL:#define _mips 1
 // MIPS32EL:#define mips 1
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding \
 // RUN:            -triple=mips64-none-none -target-abi n32 < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix MIPSN32BE -check-prefix MIPSN32BE-C %s

 // RUN:   | FileCheck -match-full-lines -check-prefix MIPS-ABS2008 %s
 // MIPS-ABS2008:#define __mips_abs2008 1
 //
 // RUN: %clang_cc1 -target-cpu mips32r3 -target-feature -abs2008 \
 // RUN:   -E -dM -triple=mips-none-none < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix NOMIPS-ABS2008 %s
 // NOMIPS-ABS2008-NOT:#define __mips_abs2008 1
 //
+// RUN: %clang_cc1  \
+// RUN:   -E -dM -triple=mips-none-none < /dev/null \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS32-NOFP %s
+// MIPS32-NOFP:#define __mips_fpr 0
+//
+// RUN: %clang_cc1 -target-feature +fpxx \
+// RUN:   -E -dM -triple=mips-none-none < /dev/null \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS32-MFPXX %s
+// MIPS32-MFPXX:#define __mips_fpr 0
+//
+// RUN: %clang_cc1 -target-cpu mips32r6 -target-feature +fpxx \
+// RUN:   -E -dM -triple=mips-none-none < /dev/null \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS32R6-MFPXX %s
+// MIPS32R6-MFPXX:#define __mips_fpr 0
+//
+// RUN: %clang_cc1  \
+// RUN:   -E -dM -triple=mips64-none-none < /dev/null \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS64-NOFP %s
+// MIPS64-NOFP:#define __mips_fpr 64
+//
+// RUN: not %clang_cc1 -target-feature -fp64 \
+// RUN:   -E -dM -triple=mips64-none-none < /dev/null 2>&1 \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS64-MFP32 %s
+// MIPS64-MFP32:error: option '-mfpxx' cannot be specified with 'mips64r2'
+//
+// RUN: not %clang_cc1 -target-feature +fpxx \
+// RUN:   -E -dM -triple=mips64-none-none < /dev/null 2>&1 \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS64-MFPXX %s
+// MIPS64-MFPXX:error: '-mfpxx' can only be used with the 'o32' ABI
+//
+// RUN: not %clang_cc1 -target-cpu mips64r6 -target-feature +fpxx \
+// RUN:   -E -dM -triple=mips64-none-none < /dev/null 2>&1 \
+// RUN:   | FileCheck -match-full-lines -check-prefix MIPS64R6-MFPXX %s
+// MIPS64R6-MFPXX:error: '-mfpxx' can only be used with the 'o32' ABI
+//
 // RUN: %clang_cc1 -target-feature -fp64 \
 // RUN:   -E -dM -triple=mips-none-none < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix MIPS32-MFP32 %s
 // MIPS32-MFP32:#define _MIPS_FPSET 16
 // MIPS32-MFP32:#define __mips_fpr 32
 //
 // RUN: %clang_cc1 -target-feature +fp64 \
 // RUN:   -E -dM -triple=mips-none-none < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix MIPS32-MFP64 %s
 // MIPS32-MFP64:#define _MIPS_FPSET 32
 // MIPS32-MFP64:#define __mips_fpr 64
 //
 // RUN: %clang_cc1 -target-feature +single-float \
 // RUN:   -E -dM -triple=mips-none-none < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix MIPS32-MFP32SF %s
 // MIPS32-MFP32SF:#define _MIPS_FPSET 32
-// MIPS32-MFP32SF:#define __mips_fpr 32
+// MIPS32-MFP32SF:#define __mips_fpr 0
 //
 // RUN: %clang_cc1 -target-feature +fp64 \
 // RUN:   -E -dM -triple=mips64-none-none < /dev/null \
 // RUN:   | FileCheck -match-full-lines -check-prefix MIPS64-MFP64 %s
 // MIPS64-MFP64:#define _MIPS_FPSET 32
 // MIPS64-MFP64:#define __mips_fpr 64
 //
 // RUN: %clang_cc1 -target-feature -fp64 -target-feature +single-float \