[Mips] Generate warning for invalid combination of '-mnan' and '-march' options.

[vradosavljevic]

Summary

This patch generates warning for invalid combination of '-mnan' and '-march' options, sets properly NaN encoding for a given '-march', and pass proper NaN encoding to the assembler.

[petarj]

lib/Basic/Targets.cpp

5729 ↗

(On Diff #21489)

According to the documentation, Has2008 field in FIR Register is optional as of R3. So, if I understand this correctly, mips32r2 always uses legacy encoding. Is that correct? If so, should we add mips32r2 case as well?

[dsanders]

lib/Basic/Targets.cpp

5729 ↗

(On Diff #21489)

That's my understanding. Although it's worth pointing out that we also accept microMIPS in mips32r2 at the moment even though it was added in mips32r3 as far as I know.

For -mnan, I think we should add mips32r2 to the legacy-only list and explain the reason to anyone who asks about it.

5962–5974 ↗

(On Diff #21489)

The diagnostics should mention that we're ignoring the request.

test/CodeGen/mips-unsupported-nan.c

8 ↗

(On Diff #21489)

We should check that the right warning is emitted for the right cases.

[atanasyan]

lib/Basic/Targets.cpp

5729 ↗

(On Diff #21489)

I think it is better to explicitly express 2008/legacy support status for each CPU. Let's consider the following code:

enum NanMode { NanLegacy = 1, Nan2008 = 2 };

NanMode getSupportedNanMode() {
    return llvm::StringSwitch<int>(CPU)
        .Case("mips1", NanLegacy)
        ....
        .Case("mips32r2", NanLegacy | Nan2008)
        ...
        .Case("mips32r6", Nan2008)
}

...

IsNan2008 = getSupportedNanMode() == Nan2008;

...

else if (*it == "+nan2008") {
  if (getSupportedNanMode() & Nan2008)
    IsNan2008 = true;
  else
    Diags.Report(diag::warn_target_unsupported_nan2008) << CPU;
}

5964 ↗

(On Diff #21489)

Why do we write a warning message right here instead to put it to the DiagnosticXXX.td file?

[vradosavljevic]

lib/Basic/Targets.cpp

5729 ↗

(On Diff #21489)

Mips32r2 and Mips64r2 are added to the legacy-only list.

5729 ↗

(On Diff #21489)

Done. Thanks for suggestion.

5962–5974 ↗

(On Diff #21489)

Done.

5964 ↗

(On Diff #21489)

Warning messages are putted in the DiagnosticCommonKinds.td file.

test/CodeGen/mips-unsupported-nan.c

8 ↗

(On Diff #21489)

Done.

[vradosavljevic]

New patch uploaded. The part of Preprocessor/init.c test is removed, because mips32r2 architecture is on the legacy-only list.

[atanasyan]

test/Misc/warning-flags.c

21 ↗

(On Diff #22028)

This change conflicts with a comment on the line #19.

test/Preprocessor/init.c

4422 ↗

(On Diff #22028)

Are there any other tests that check __mips_nan2008 macro generation?

[petarj]

lib/Basic/Targets.cpp

5769 ↗

(On Diff #22028)

It might be me only, but seeing a function that returns enum but can actually return non-enumerated value seems wrong (I am not saying it is illegal though).
I see this is already done for ArchDefineTypes within PPCTargetInfo, but it still looks wrong (to me).
Would not this trigger a runtime error in clang if it was compiled with "-fsanitize=enum"?

[vradosavljevic]

test/Preprocessor/init.c

4422 ↗

(On Diff #22028)

In the same test there is a case that check __mips_nan2008 macro.

[vradosavljevic]

New patch uploaded.

[dsanders]

lib/Basic/Targets.cpp

5769 ↗

(On Diff #22028)

I think this is the main remaining comment. I agree in principle that enum types should have the value of one of the enum values. Like Petar, I don't think it's technically wrong though. Have you tried the -fsanitize=enum option that was suggested?

Elsewhere, an integer type is used instead but given that there's only two values and I would be very surprised if we ever add more, we should probably just define NanBoth = 3 if it's a problem.

lib/Driver/Tools.cpp

1119–1121 ↗

(On Diff #22884)

Style nit: Unecessary braces around single-line if-statement.

1126–1128 ↗

(On Diff #22884)

Style nit: Unecessary braces around single-line if-statement.

test/Preprocessor/init.c

4422 ↗

(On Diff #22028)

For reference: It's on line 4457 with the patch applied.

[atanasyan]

test/Preprocessor/init.c

4419 ↗

(On Diff #22884)

By the way, why do you decide to remove that test at all? Does it fail now?

[vradosavljevic]

lib/Basic/Targets.cpp

5769 ↗

(On Diff #22028)

I tried -fsanitize=enum there wasn't runtime error. Should i define NanBoth?

test/Preprocessor/init.c

4419 ↗

(On Diff #22884)

It doesn't fail now, but is wrong to pass +nan2008 with the default cpu (mips32r2), because mips32r2 architecture is on the legacy-only list.

[atanasyan]

test/Preprocessor/init.c

4419 ↗

(On Diff #22884)

Let's pass a correct set of arguments and check that +nan2008 switches on the __mips_nan2008 macro. And maybe additionally check that -nan2008 with the same options switches the macro off.

[petarj]

lib/Basic/Targets.cpp

5769 ↗

(On Diff #22028)

It seems -fsanitize=enum will allow all values up to a number that you get when you OR all possible valid enum values, so 3 in this case is OK, but not 4 for instance.
So, no need to modify this if none of the tools is complaining.

[vradosavljevic]

lib/Driver/Tools.cpp

1119–1121 ↗

(On Diff #22884)

Done.

1126–1128 ↗

(On Diff #22884)

Done.

test/Preprocessor/init.c

4419 ↗

(On Diff #22884)

Done.

[vradosavljevic]

Comments addressed.

[atanasyan]

LGTM

cfe/trunk/include/clang/Basic/DiagnosticDriverKinds.td

def err_analyzer_config_multiple_values : Error<
   "analyzer-config option '%0' should contain only one '='">;
 
 def err_drv_modules_validate_once_requires_timestamp : Error<
   "option '-fmodules-validate-once-per-build-session' requires "
   "'-fbuild-session-timestamp=<seconds since Epoch>' or '-fbuild-session-file=<file>'">;
 
 def warn_drv_invoking_fallback : Warning<"falling back to %0">,
   InGroup<Fallback>;
+
+def warn_target_unsupported_nan2008 : Warning<
+  "ignoring '-mnan=2008' option because the '%0' architecture does not support it">,
+  InGroup<UnsupportedNan>;
+def warn_target_unsupported_nanlegacy : Warning<
+  "ignoring '-mnan=legacy' option because the '%0' architecture does not support it">,
+  InGroup<UnsupportedNan>;
 }

cfe/trunk/include/clang/Basic/DiagnosticGroups.td

 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;
 def UndefinedBoolConversion : DiagGroup<"undefined-bool-conversion">;
 def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion,
                                                    UndefinedBoolConversion]>;
 def IntConversion : DiagGroup<"int-conversion">;
 def EnumConversion : DiagGroup<"enum-conversion">;
 def FloatConversion : DiagGroup<"float-conversion">;
 def EnumTooLarge : DiagGroup<"enum-too-large">;
+def UnsupportedNan : DiagGroup<"unsupported-nan">;
 def NonLiteralNullConversion : DiagGroup<"non-literal-null-conversion">;
 def NullConversion : DiagGroup<"null-conversion">;
 def ImplicitConversionFloatingPointToBool :
   DiagGroup<"implicit-conversion-floating-point-to-bool">;
 def ObjCLiteralConversion : DiagGroup<"objc-literal-conversion">;
 def BadArrayNewLength : DiagGroup<"bad-array-new-length">;
 def MacroRedefined : DiagGroup<"macro-redefined">;
 def BuiltinMacroRedefined : DiagGroup<"builtin-macro-redefined">;

cfe/trunk/lib/Driver/Tools.h

namespace arm {
   const char* getARMCPUForMArch(const llvm::opt::ArgList &Args,
                                 const llvm::Triple &Triple);
   const char* getLLVMArchSuffixForARM(StringRef CPU);
 
   void appendEBLinkFlags(const llvm::opt::ArgList &Args, ArgStringList &CmdArgs, const llvm::Triple &Triple);
 }
 
 namespace mips {
+  typedef enum {
+    NanLegacy = 1,
+    Nan2008 = 2
+  } NanEncoding;
+  NanEncoding getSupportedNanEncoding(StringRef &CPU);
   void getMipsCPUAndABI(const llvm::opt::ArgList &Args,
                         const llvm::Triple &Triple, StringRef &CPUName,
                         StringRef &ABIName);
   bool hasMipsAbiArg(const llvm::opt::ArgList &Args, const char *Value);
   bool isUCLibc(const llvm::opt::ArgList &Args);
   bool isNaN2008(const llvm::opt::ArgList &Args, const llvm::Triple &Triple);
   bool isFPXXDefault(const llvm::Triple &Triple, StringRef CPUName,
                      StringRef ABIName);

cfe/trunk/lib/Driver/Tools.cpp

if (FloatABI == "soft") {
     // FIXME: Note, this is a hack. We need to pass the selected float
     // mode to the MipsTargetInfoBase to define appropriate macros there.
     // Now it is the only method.
     Features.push_back("+soft-float");
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_mnan_EQ)) {
     StringRef Val = StringRef(A->getValue());
-    if (Val == "2008")
+    if (Val == "2008") {
+      if (mips::getSupportedNanEncoding(CPUName) & mips::Nan2008)
         Features.push_back("+nan2008");
-    else if (Val == "legacy")
+      else {
         Features.push_back("-nan2008");
-    else
+        D.Diag(diag::warn_target_unsupported_nan2008) << CPUName;
+      }
+    } else if (Val == "legacy") {
+      if (mips::getSupportedNanEncoding(CPUName) & mips::NanLegacy)
+        Features.push_back("-nan2008");
+      else {
+        Features.push_back("+nan2008");
+        D.Diag(diag::warn_target_unsupported_nanlegacy) << CPUName;
+      }
+    } else
       D.Diag(diag::err_drv_unsupported_option_argument)
           << A->getOption().getName() << Val;
   }
 
   AddTargetFeature(Args, Features, options::OPT_msingle_float,
                    options::OPT_mdouble_float, "single-float");
   AddTargetFeature(Args, Features, options::OPT_mips16, options::OPT_mno_mips16,
                    "mips16");
const char *LinkFlag = llvm::StringSwitch<const char *>(Suffix)
     .Cases("v4", "v4t", "v5", "v5e", nullptr)
     .Cases("v6", "v6k", "v6t2", nullptr)
     .Default("--be8");
 
   if (LinkFlag)
     CmdArgs.push_back(LinkFlag);
 }
 
+mips::NanEncoding mips::getSupportedNanEncoding(StringRef &CPU) {
+  return (NanEncoding)llvm::StringSwitch<int>(CPU)
+      .Case("mips1", NanLegacy)
+      .Case("mips2", NanLegacy)
+      .Case("mips3", NanLegacy)
+      .Case("mips4", NanLegacy)
+      .Case("mips5", NanLegacy)
+      .Case("mips32", NanLegacy)
+      .Case("mips32r2", NanLegacy)
+      .Case("mips32r3", NanLegacy | Nan2008)
+      .Case("mips32r5", NanLegacy | Nan2008)
+      .Case("mips32r6", Nan2008)
+      .Case("mips64", NanLegacy)
+      .Case("mips64r2", NanLegacy)
+      .Case("mips64r3", NanLegacy | Nan2008)
+      .Case("mips64r5", NanLegacy | Nan2008)
+      .Case("mips64r6", Nan2008)
+      .Default(NanLegacy);
+}
+
 bool mips::hasMipsAbiArg(const ArgList &Args, const char *Value) {
   Arg *A = Args.getLastArg(options::OPT_mabi_EQ);
   return A && (A->getValue() == StringRef(Value));
 }
 
 bool mips::isUCLibc(const ArgList &Args) {
   Arg *A = Args.getLastArg(options::OPT_m_libc_Group);
   return A && A->getOption().matches(options::OPT_muclibc);

cfe/trunk/test/CodeGen/mips-unsupported-nan.c

+// RUN: %clang -target mipsel-unknown-linux -mnan=2008 -march=mips2 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS2 -check-prefix=CHECK-NANLEGACY %s
+// RUN: %clang -target mips64el-unknown-linux -mnan=2008 -march=mips3 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS3 -check-prefix=CHECK-NANLEGACY %s
+// RUN: %clang -target mips64el-unknown-linux -mnan=2008 -march=mips4 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS4 -check-prefix=CHECK-NANLEGACY %s
+// RUN: %clang -target mipsel-unknown-linux -mnan=2008 -march=mips32 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS32 -check-prefix=CHECK-NANLEGACY %s
+// RUN: %clang -target mipsel-unknown-linux -mnan=2008 -march=mips32r2 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS32R2 -check-prefix=CHECK-NANLEGACY %s
+// RUN: %clang -target mipsel-unknown-linux -mnan=2008 -march=mips32r3 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-NOT-MIPS32R3 -check-prefix=CHECK-NAN2008 %s
+// RUN: %clang -target mipsel-unknown-linux -mnan=legacy -march=mips32r6 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS32R6 -check-prefix=CHECK-NAN2008 %s
+// RUN: %clang -target mips64el-unknown-linux -mnan=2008 -march=mips64 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS64 -check-prefix=CHECK-NANLEGACY %s
+// RUN: %clang -target mips64el-unknown-linux -mnan=2008 -march=mips64r2 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS64R2 -check-prefix=CHECK-NANLEGACY %s
+// RUN: %clang -target mips64el-unknown-linux -mnan=legacy -march=mips64r6 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-MIPS64R6 -check-prefix=CHECK-NAN2008 %s
+
+// CHECK-MIPS2: warning: ignoring '-mnan=2008' option because the 'mips2' architecture does not support it
+// CHECK-MIPS3: warning: ignoring '-mnan=2008' option because the 'mips3' architecture does not support it
+// CHECK-MIPS4: warning: ignoring '-mnan=2008' option because the 'mips4' architecture does not support it
+// CHECK-MIPS32: warning: ignoring '-mnan=2008' option because the 'mips32' architecture does not support it
+// CHECK-MIPS32R2: warning: ignoring '-mnan=2008' option because the 'mips32r2' architecture does not support it
+// CHECK-MIPS32R3: warning: ignoring '-mnan=2008' option because the 'mips32r3' architecture does not support it
+// CHECK-MIPS32R6: warning: ignoring '-mnan=legacy' option because the 'mips32r6' architecture does not support it
+// CHECK-MIPS64: warning: ignoring '-mnan=2008' option because the 'mips64' architecture does not support it
+// CHECK-MIPS64R2: warning: ignoring '-mnan=2008' option because the 'mips64r2' architecture does not support it
+// CHECK-MIPS64R6: warning: ignoring '-mnan=legacy' option because the 'mips64r6' architecture does not support it
+// CHECK-NANLEGACY: float 0x7FF4000000000000
+// CHECK-NAN2008: float 0x7FF8000000000000
+
+float f =  __builtin_nan("");

cfe/trunk/test/Driver/mips-features.c

 //
 // -mfp32
 // RUN: %clang -target mips-linux-gnu -### -c %s \
 // RUN:     -mfp64 -mfp32 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NOMFP64 %s
 // CHECK-NOMFP64: "-target-feature" "-fp64"
 //
 // -mnan=2008
-// RUN: %clang -target mips-linux-gnu -### -c %s \
+// RUN: %clang -target mips-linux-gnu -march=mips32r3 -### -c %s \
 // RUN:     -mnan=legacy -mnan=2008 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NAN2008 %s
 // CHECK-NAN2008: "-target-feature" "+nan2008"
 //
 // -mnan=legacy
-// RUN: %clang -target mips-linux-gnu -### -c %s \
+// RUN: %clang -target mips-linux-gnu -march=mips32r3 -### -c %s \
 // RUN:     -mnan=2008 -mnan=legacy 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NANLEGACY %s
 // CHECK-NANLEGACY: "-target-feature" "-nan2008"
 //
 // -mxgot
 // RUN: %clang -target mips-linux-gnu -### -c %s \
 // RUN:     -mno-xgot -mxgot 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-XGOT %s

cfe/trunk/test/Driver/mips-integrated-as.s

 // NAN-DEFAULT: -cc1as
 // NAN-DEFAULT-NOT: "-target-feature" "{{[-+]}}nan2008"
 
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mnan=legacy 2>&1 | \
 // RUN:   FileCheck -check-prefix=NAN-LEGACY %s
 // NAN-LEGACY: -cc1as
 // NAN-LEGACY: "-target-feature" "-nan2008"
 
-// RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s -mnan=2008 2>&1 | \
+// RUN: %clang -target mips-linux-gnu -march=mips32r6 -### -fintegrated-as -c %s -mnan=2008 2>&1 | \
 // RUN:   FileCheck -check-prefix=NAN-2008 %s
 // NAN-2008: -cc1as
 // NAN-2008: "-target-feature" "+nan2008"
 
 // RUN: %clang -target mips-linux-gnu -### -fintegrated-as -c %s 2>&1 | \
 // RUN:   FileCheck -check-prefix=DEFAULT-FLOAT %s
 // DEFAULT-FLOAT: -cc1as
 // DEFAULT-FLOAT-NOT: "-target-feature" "{{[+-]}}single-float"

cfe/trunk/test/Preprocessor/init.c

 // MIPS-DSPR2:#define __mips_dsp_rev 2
 // MIPS-DSPR2:#define __mips_dspr2 1
 //
 // RUN: %clang_cc1 -target-feature +msa \
 // RUN:   -E -dM -triple=mips-none-none < /dev/null \
 // RUN:   | FileCheck -check-prefix MIPS-MSA %s
 // MIPS-MSA:#define __mips_msa 1
 //
-// RUN: %clang_cc1 -target-feature +nan2008 \
+// RUN: %clang_cc1 -target-cpu mips32r3 -target-feature +nan2008 \
 // RUN:   -E -dM -triple=mips-none-none < /dev/null \
 // RUN:   | FileCheck -check-prefix MIPS-NAN2008 %s
 // MIPS-NAN2008:#define __mips_nan2008 1
 //
+// RUN: %clang_cc1 -target-cpu mips32r3 -target-feature -nan2008 \
+// RUN:   -E -dM -triple=mips-none-none < /dev/null \
+// RUN:   | FileCheck -check-prefix NOMIPS-NAN2008 %s
+// NOMIPS-NAN2008-NOT:#define __mips_nan2008 1
+//
 // RUN: %clang_cc1 -target-feature -fp64 \
 // RUN:   -E -dM -triple=mips-none-none < /dev/null \
 // RUN:   | FileCheck -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 \