Page MenuHomePhabricator

[Driver][ARM] Disable unsupported features when nofp arch extension is used
ClosedPublic

Authored by vhscampos on Jul 1 2020, 4:28 AM.

Details

Summary

A list of target features is disabled when there is no hardware
floating-point support. This is the case when one of the following
options is passed to clang:

  • -mfloat-abi=soft
  • -mfpu=none

This option list is missing, however, the extension "+nofp" that can be
specified in -march flags, such as "-march=armv8-a+nofp".

This patch also disables unsupported target features when nofp is passed
to -march.

Diff Detail

Event Timeline

vhscampos created this revision.Jul 1 2020, 4:28 AM
chill added a comment.Jul 1 2020, 12:24 PM

Needs a regression test. This patch and the dependent patch clash, better with a single patch.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
289

That's kinda mouthful name.

293–298

Wouldn't just looking for the substring do the job?

Also need to handle -mcpu=...+nofp.

We already "parse" the arguments to -march= and -mcpu= (and -mfpu=) earlier, it seems to me we
could note the +nofp and +nofp.dp earlier. (TBH, it isn't immediately obvious to me how to untangle this mess).

chill added inline comments.Jul 2 2020, 12:58 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
293–298

Hmm, actually, +nofp.dp should not disable the FPU, I think.

DavidSpickett added inline comments.Jul 2 2020, 1:34 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
297

I would check what this does:
$ ./bin/clang -target arm-arm-none-eabi /tmp/test.c -c -march=armv8.1-a+nofp+fp -###

I'm not sure if by this point we've resolved "+nofp+fp" to simply "+fp". (perhaps you'd end up with a bunch of "-<feature>" followed by the same as "+<feature>". Either way I'd expect +fp to win.

vhscampos marked 2 inline comments as done.Jul 2 2020, 1:35 AM

I will merge the two patches into one.

Please also see my inline responses.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
289

Not sure how to compress it more without making it unmeaningful though.

293–298

Just looking for the substring might be sufficient indeed.

Yes, we already do -march/-mcpu parsing a bit earlier. However, this parsing and the following handling of it is done deeper in the call stack. I wondered about ways to propagate this information back to this point here (e.g. adding one more by-ref argument that is set by the first round of parsing), but I don't feel confident to back it up.

Are you okay with me just changing it to a substring search?

vhscampos marked 2 inline comments as done.Jul 2 2020, 6:37 AM
vhscampos added inline comments.
clang/lib/Driver/ToolChains/Arch/ARM.cpp
293–298

Actually it may be better to keep the string splitting method. The search required here must be whole-word, as to flag up "+nofp", but not "+nofp.dp". It can be done with less code using the current list of tokens as opposed to using substring search, followed by a "is it whole-word?" check.

297

Good catch. I will be sure to cover this case.

chill added inline comments.Jul 2 2020, 8:12 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
293–298

In fact, it's less number of tokens, not that number of tokens is that important.

auto anyNOFP = [](const StringRef &str) {
  size_t pos = str.find_lower("+nofp");
  return pos != StringRef::npos &&
          (pos + 5 == str.size() || str[pos + 5] == '+');
};

Or it could become

auto findFeature = [](const StringRef &str, const StringRef &feature) {
  size_t pos = str.find_lower(feature);
  return (pos == StringRef::npos || pos + feature.size() == str.size() ||
          str[pos + feature.size()] == '+')
             ? pos
             : StringRef::npos;
};

which is slightly longer, but more universal and allows you to check relative positions of features.

A-a-a-nyway, I think this string twiddling should be the absolute last resort.

Could you please, see, if checkARMArchName and checkARMCpuName can be tweaked into
returning llvm::ARM::FK_NONE or llvm::ARM::FK_INVALID or whatever, depending on what is present
in option value of -march=... and -mcpu=...?

An interesting question is what to do if there are contradictions, but I think the general strategy is to not strive to produce sensible output from nonsensical input.

vhscampos updated this revision to Diff 275392.Jul 3 2020, 7:39 AM
  1. Merged the second patch into this (handle bf16).
  2. Do the same treatment for -mcpu.
  3. Instead of doing string search once again, return the desired information in the first time using a by-ref argument.
  4. This new approach covers positional differences between +fp and +nofp.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2020, 7:39 AM
DavidSpickett added inline comments.Jul 21 2020, 5:24 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
476

Why not disable MVE-I? I assume because it's integer only but then why does -mfloat-abi=soft disable it?

If possible add a regression test for this. In general a test like the bf16 test below, but for all the listed extensions would help. Perhaps it makes more sense to add a driver test that looks for the "-<ext>" bits in the -### output instead of doing each extension on its own.

chill added inline comments.Jul 21 2020, 5:45 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
476

Why not disable MVE-I?

After MVE, "FPU" registers are a separate entity from the FPU.

-mfpu=none/+nofp disable the FPU. MVE-I does not require an FPU.
-mfloat-abi=soft disables both the FPU instructions and the FPU registers.
MVE-I requires "FPU" registers.

It's possible to define different semantics, but this is the one we agreed with GCC.

DavidSpickett added inline comments.Jul 22 2020, 1:18 AM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
476

Got it. @vhscampos can you add that to the comment?

vhscampos updated this revision to Diff 280072.Jul 23 2020, 4:20 AM
  1. Add comment explaining the MVE-Integer detail.
  2. Add another test to check the disabled features.
DavidSpickett marked an inline comment as done.Jul 23 2020, 5:11 AM

LGTM, I'll let @chill give the final word.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
476

Thanks, lgtm.

chill accepted this revision.Jul 29 2020, 3:25 AM

LGTM, thanks!

clang/lib/Driver/ToolChains/Arch/ARM.cpp
482

LLVM coding standards call for not using braces on single-statement bodies and that's also the style in this source file.

This revision is now accepted and ready to land.Jul 29 2020, 3:25 AM
This revision was landed with ongoing or failed builds.Jul 29 2020, 6:19 AM
This revision was automatically updated to reflect the committed changes.