[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

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

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

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

test/CodeGen/mips-unsupported-nan.c

8

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

[atanasyan]

lib/Basic/Targets.cpp

5729

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

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

[vradosavljevic]

lib/Basic/Targets.cpp

5729

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

5729

Done. Thanks for suggestion.

5962–5974

Done.

5964

Warning messages are putted in the DiagnosticCommonKinds.td file.

test/CodeGen/mips-unsupported-nan.c

8

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

5729

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

5729

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

5729

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

5729

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

lib/Basic/Targets.cpp

public:
   MipsTargetInfoBase(const llvm::Triple &Triple, const std::string &ABIStr,
                      const std::string &CPUStr)
       : TargetInfo(Triple), CPU(CPUStr), IsMips16(false), IsMicromips(false),
         IsNan2008(false), IsSingleFloat(false), FloatABI(HardFloat),
         DspRev(NoDSP), HasMSA(false), HasFP64(false), ABI(ABIStr) {
     TheCXXABI.set(TargetCXXABI::GenericMIPS);
   }
 
-  bool isNaN2008Default() const {
+  bool hasOnlyNaN2008Encoding() const {
     return CPU == "mips32r6" || CPU == "mips64r6";
   }
 
+  bool hasOnlyNaNLegacyEncoding() const {
+    return llvm::StringSwitch<bool>(CPU)
+        .Case("mips1", true)
+        .Case("mips2", true)
+        .Case("mips3", true)
+        .Case("mips4", true)
+        .Case("mips5", true)
+        .Case("mips32", true)
+        .Case("mips64", true)
+        .Default(false);
+  }
+
   bool isFP64Default() const {
     return CPU == "mips32r6" || ABI == "n32" || ABI == "n64" || ABI == "64";
   }
 
   bool isNan2008() const override {
     return IsNan2008;
   }
 
const char *getClobbers() const override {
     // assembly operands if the user's assembly code doesn't use it.
     return "~{$1}";
   }
 
   bool handleTargetFeatures(std::vector<std::string> &Features,
                             DiagnosticsEngine &Diags) override {
     IsMips16 = false;
     IsMicromips = false;
-    IsNan2008 = isNaN2008Default();
+    IsNan2008 = hasOnlyNaN2008Encoding();
     IsSingleFloat = false;
     FloatABI = HardFloat;
     DspRev = NoDSP;
     HasFP64 = isFP64Default();
 
     for (std::vector<std::string>::iterator it = Features.begin(),
          ie = Features.end(); it != ie; ++it) {
       if (*it == "+single-float")
for (std::vector<std::string>::iterator it = Features.begin(),
       else if (*it == "+dspr2")
         DspRev = std::max(DspRev, DSP2);
       else if (*it == "+msa")
         HasMSA = true;
       else if (*it == "+fp64")
         HasFP64 = true;
       else if (*it == "-fp64")
         HasFP64 = false;
-      else if (*it == "+nan2008")
+      else if (*it == "+nan2008") {
+        if (hasOnlyNaNLegacyEncoding()) {
+          unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Warning,
+                    "the '%0' architecture does not support '-mnan=2008'");
+          Diags.Report(DiagID) << CPU;
+        } else
           IsNan2008 = true;
-      else if (*it == "-nan2008")
+      } else if (*it == "-nan2008") {
+        if (hasOnlyNaN2008Encoding()) {
+          unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Warning,
+                    "the '%0' architecture does not support '-mnan=legacy'");
+          Diags.Report(DiagID) << CPU;
+        } else
           IsNan2008 = false;
       }
+    }
 
     // Remove front-end specific options.
     std::vector<std::string>::iterator it =
       std::find(Features.begin(), Features.end(), "+soft-float");
     if (it != Features.end())
       Features.erase(it);
 
     setDescriptionString();

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-WARNING -check-prefix=CHECK-LEGACY %s
+// RUN: %clang -target mipsel-unknown-linux -mnan=2008 -march=mips32 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-WARNING -check-prefix=CHECK-LEGACY %s
+// RUN: %clang -target mips64el-unknown-linux -mnan=2008 -march=mips64 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-WARNING -check-prefix=CHECK-LEGACY %s
+// RUN: %clang -target mipsel-unknown-linux -mnan=legacy -march=mips32r6 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-WARNING -check-prefix=CHECK-2008 %s
+// RUN: %clang -target mips64el-unknown-linux -mnan=legacy -march=mips64r6 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-WARNING -check-prefix=CHECK-2008 %s
+// RUN: %clang -target mipsel-unknown-linux -mnan=legacy -march=mips32r2 -emit-llvm -S %s -o - 2>&1 | FileCheck -check-prefix=CHECK-NOT-WARNING -check-prefix=CHECK-LEGACY %s
+
+// CHECK-WARNING: warning: the '{{mips.*}}' architecture does not support '-mnan={{2008|legacy}}'
+// CHECK-LEGACY: float 0x7FF4000000000000
+// CHECK-2008: float 0x7FF8000000000000
+
+float f =  __builtin_nan("");