Page MenuHomePhabricator

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

Authored by vradosavljevic on Mar 9 2015, 8:24 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

vradosavljevic retitled this revision from to [Mips] Generate warning for invalid combination of '-mnan' and '-march' options..
vradosavljevic updated this object.
vradosavljevic edited the test plan for this revision. (Show Details)
vradosavljevic added reviewers: dsanders, petarj.
vradosavljevic added a subscriber: Unknown Object (MLST).
petarj added inline comments.Mar 9 2015, 11:19 AM
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 added inline comments.Mar 13 2015, 10:23 AM
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 added inline comments.
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 added inline comments.Mar 16 2015, 9:08 AM
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.

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

atanasyan added inline comments.Mar 16 2015, 10:37 AM
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 added inline comments.Mar 16 2015, 10:56 AM
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 added inline comments.Mar 30 2015, 7:38 AM
test/Preprocessor/init.c
4422 ↗(On Diff #22028)

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

vradosavljevic updated this object.

New patch uploaded.

dsanders added inline comments.Apr 1 2015, 12:07 PM
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 added inline comments.Apr 2 2015, 2:58 AM
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 added inline comments.Apr 2 2015, 4:42 AM
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 added inline comments.Apr 2 2015, 5:04 AM
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 added inline comments.Apr 7 2015, 6:59 AM
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 added inline comments.Apr 8 2015, 2:07 AM
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.

Comments addressed.

atanasyan accepted this revision.Apr 8 2015, 6:02 AM
atanasyan added a reviewer: atanasyan.

LGTM

This revision is now accepted and ready to land.Apr 8 2015, 6:02 AM
This revision was automatically updated to reflect the committed changes.