Page MenuHomePhabricator

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

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



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.


That's kinda mouthful name.


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

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

DavidSpickett added inline comments.Jul 2 2020, 1:34 AM

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.


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


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.

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.


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

chill added inline comments.Jul 2 2020, 8:12 AM

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

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

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

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.


Thanks, lgtm.

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

LGTM, thanks!


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.