Page MenuHomePhabricator

[ARM] Replace fp-only-sp and d16 with fp64 and d32.
Needs ReviewPublic

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

Details

Summary

Those two subtarget features were awkward because their semantics are
reversed: each one indicates the _lack_ of support for something in
the architecture, rather than the presence. As a consequence, you
don't get the behavior you want if you combine two sets of feature
bits.

Each SubtargetFeature for an FP architecture version now comes in four
versions, one for each combination of those options. So you can still
say (for example) '+vfp2' in a feature string and it will mean what
it's always meant, but there's a new string '+vfp2d16sp' meaning the
version without those extra options.

A lot of this change is just mechanically replacing positive checks
for the old features with negative checks for the new ones. But one
more interesting change is that I've rearranged getFPUFeatures() so
that the main FPU feature is appended to the output list *before*
rather than after the features derived from the Restriction field, so
that -fp64 and -d32 can override defaults added by the main feature.

Event Timeline

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

I like the direction of this change, and the details look correct too.

The one thing I wonder about is whether we should be upgrading .bc files too (or otherwise support fp-only-sp in legacy inputs). I think it's a specialized enough feature that there won't be much older IR being mixed in, but can't be sure there's none. Anyone else got any views?

ostannard edited reviewers, added: ostannard; removed: olista01.Apr 17 2019, 9:19 AM
ostannard added a subscriber: ostannard.

For the auto-upgrader, these limited FPUs only exist for microcontrollers, where I doubt any users are keeping bitcode files around for a long time, so I'd be fine with not doing this. I've had a skim through the auto-upgrader code, and I don't see any other target-specific attributes which are handled there, though there are some target-specific intrinsics.

I don't think we here care about auto-updating, not supporting bitcode/lto libraries.

llvm/lib/Target/ARM/ARMFastISel.cpp
1362

Some of the formatting here got a little messed up.

ostannard added inline comments.Fri, May 3, 6:08 AM
clang/test/Driver/arm-mfpu.c
112

"+d32" ?

llvm/lib/Target/ARM/ARMSubtarget.h
587

Are the old functions still used anywhere? If they are not used (much), I think it would be better to just have one set of functions for the base FPU version, and check hasFP64 and hasD32 where needed, to avoid the rick of using the wrong version of these functions.

llvm/test/MC/ARM/armv8.3a-js.s
16

Do you know why this diagnostic got worse?