Page MenuHomePhabricator

[ARM] Allow "-march=foo+fp" to vary with foo.
Needs ReviewPublic

Authored by simon_tatham on Apr 15 2019, 5:57 AM.

Details

Summary

Now, when clang processes an argument of the form "-march=foo+x+y+z",
then instead of calling getArchExtFeature() for each of the extension
names "x", "y", "z" and appending the returned string to its list of
low-level subtarget features, it will call appendArchExtFeatures()
which does the appending itself.

The difference is that appendArchExtFeatures can add _more_ than one
low-level feature name to the output feature list if it has to, and
also, it gets told some information about what base architecture and
CPU the extension is going to go with, which means that "+fp" can now
mean something different for different CPUs. Namely, "+fp" now selects
whatever the _default_ FPU is for the selected CPU and/or
architecture, as defined in the ARM_ARCH or ARM_CPU_NAME macros in
ARMTargetParser.def.

On the clang side, I adjust DecodeARMFeatures to call the new
appendArchExtFeatures function in place of getArchExtFeature. This
means DecodeARMFeatures needs to be passed a CPU name and an ArchKind,
which meant changing its call sites to make those available, and also
sawing getLLVMArchSuffixForARM in half so that you can get an ArchKind
enum value out of it instead of a string.

Also, I add support here for the extension name "+fp.dp", which will
automatically look through the FPU list for something that looks just
like the default FPU except for also supporting double precision.

Event Timeline

simon_tatham created this revision.Apr 15 2019, 5:57 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 15 2019, 5:57 AM

This needs some tests. I'm also not quite sure when you'd use bare "+fp", if it's the default anyway.

llvm/lib/Support/ARMTargetParser.cpp
476

I think isNegated is probably more in line with existing naming.

504–507

Doesn't this mean +nofp.dp disables all floats? That seems surprising behaviour to me, but I can't find any existing tests covering it.

516–517

Doesn't this silently disable the FPU entirely if we decide "fp.dp" is useless? That seems unlikely to be what a user wants, especially without a diagnostic.

Could you also expand on the comment a bit more. I had to look up exactly what FPURestrictions existed to get this, and I'm not even 100% sure I'm right now.

simon_tatham marked 3 inline comments as done.Apr 16 2019, 2:53 AM

The aim of this change is that it will apply to the v8.1-M (mainline) architecture introduced in D60698, in which +fp won't be the default: -march=armv8.1m.main by itself gives you the base 8.1-M architecture without any FP, -march=armv8.1m.main+fp gives you the optional single-precision FP extension on top of that, and +fp.dp gives you double precision as well.

llvm/lib/Support/ARMTargetParser.cpp
476

Hmmm. I thought that would be a confusing name because it hides the fact that the function strips off the no prefix. (The use of 'was' was intended to hint that by the time the function returns, it's not true any more!)

Perhaps stripNegationPrefix (returning bool to indicate success)?

504–507

Hmmm, that's a good point. What would a user expect in that situation? If double-precision FP was the default for that architecture and a single-precision version existed, then perhaps nofp.dp should fall back to that, but what if it's double or nothing?

516–517

I don't think it silently disables it, does it? Returning false from this function is a failure indication that ends up back in checkARMArchName in clang/lib/Driver/ToolChains/Arch/ARM.cpp, which will generate a diagnostic. For example, if I try -march=armv6m+fp.dp then I see

error: the clang compiler does not support '-march=armv6m+fp.dp'
t.p.northover added inline comments.Apr 16 2019, 3:15 AM
llvm/lib/Support/ARMTargetParser.cpp
476

Ah yes, I see. I think your alternative is probably better.

504–507

I think I'd go for a diagnostic in that case. There's already a way to strip out the FPU then (+nofp).

516–517

Ah, I missed the only way return true could happen and assumed the return value was vestigial. Sorry.