This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Add knowledge of FPU subtarget features to TargetParser
ClosedPublic

Authored by john.brawn on Jun 4 2015, 4:17 AM.

Details

Summary

Add getFPUFeatures to TargetParser, which gets the list of subtarget features that are enabled/disabled for each FPU, and use it when handling the .fpu directive.

No functional change in this commit, though clang will start behaving differently once it starts using this.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn updated this revision to Diff 27102.Jun 4 2015, 4:17 AM
john.brawn retitled this revision from to [ARM] Add knowledge of FPU subtarget features to TargetParser.
john.brawn updated this object.
john.brawn edited the test plan for this revision. (Show Details)
john.brawn added a reviewer: rengolin.
john.brawn set the repository for this revision to rL LLVM.
john.brawn added a subscriber: Unknown Object (MLST).
rengolin edited edge metadata.Jun 4 2015, 5:49 AM

Hi John,

Thanks for working on this.

I think this patch could be split into smaller chunks, and move other chunks elsewhere.

One patch is to add the enums, update the table and create the get methods. No <vector>.

Another is to move the parsing logic from ARMAsmParser to a new method, one outside the ARMTargetParser and ARMAsmParser, that uses a Features vector and common both assembler's and Clang's feature bits parsing, but only changes in the assembler side (LLVM patch). ARMSubTarget is probably the best place for it.

The final patch (D10239), will then just move to this new method.

Makes sense?

cheers,
--renato

include/llvm/Support/TargetParser.h
18 ↗(On Diff #27102)

This will include <vector> is a lot of other files that don't need it. Can you use SmallVector or something and declare it like StringRef?

lib/Support/TargetParser.cpp
27 ↗(On Diff #27102)

I'm leaving enums in the header file for now. It may not be pretty to expose the internal of it for now, but we need to understand where things are used and why, so that when we implement the new Tuple class (see the list for more details), we can merge enums from Triple, TargetParser and others into one big, internal, implementation detail.

253 ↗(On Diff #27102)

I think this method is too specific, and it should be implemented in the user's side.

The overall design of this class must be as simple as possible, even if that means more code on the user side. So, create two new methods getRestriction() and getNeonSupport() by using the two new columns provided and implement the rest in the caller's side.

257 ↗(On Diff #27102)

No need for brackets here.

Another is to move the parsing logic from ARMAsmParser to a new method, one outside the ARMTargetParser and ARMAsmParser, that uses a Features vector and common both assembler's and Clang's feature bits parsing, but only changes in the assembler side (LLVM patch). ARMSubTarget is probably the best place for it.

The problem with moving that part to ARMSubTarget is that then clang can't see it, as nothing in lib/Target/TargetName is exposed in include/llvm/Target. I could maybe put something in MC/SubtargetFeature or MC/MCSubtargetInfo, but that would mean ARM-specific stuff is polluting a target-independent interface unless I do something like add a target-independent TargetParser layer on top of ARMTargetParser.

include/llvm/Support/TargetParser.h
18 ↗(On Diff #27102)

std::vector is the standard way of handling lists of subtarget feature flags in clang, so using something else would make the interface with clang messier. Also there are quite a lot of headers that include <vector>, and looking at the places TargetParser is used they're probably including one of them already.

What about moving it to Triple?

What about moving it to Triple?

That would be a bit of an odd place to do it, as FPUs and triples only indirectly have anything to do with each other - a triple may indirectly cause a default FPU due to the default CPU, but there's no link in the FPU->Triple direction.

It's not clear to my why TargetParser isn't the right place for this. You say "The overall design of this class must be as simple as possible", but I don't see why that is. My impression of TargetParser is that it handles target-specific names in a way that means the thing using it doesn't need target-specific information, e.g. you can go from architecture name to default CPU name without needing to know what those names mean. Going from FPU name to a list of subtarget feature flags doesn't seem that different to me.

rengolin accepted this revision.Jun 4 2015, 10:04 AM
rengolin edited edge metadata.

That would be a bit of an odd place to do it, as FPUs and triples only indirectly have anything to do with each other - a triple may indirectly cause a default FPU due to the default CPU, but there's no link in the FPU->Triple direction.

That's a good point.

It's not clear to my why TargetParser isn't the right place for this. You say "The overall design of this class must be as simple as possible", but I don't see why that is. My impression of TargetParser is that it handles target-specific names in a way that means the thing using it doesn't need target-specific information, e.g. you can go from architecture name to default CPU name without needing to know what those names mean. Going from FPU name to a list of subtarget feature flags doesn't seem that different to me.

Now I'm less convinced that this doesn't belong there, but I really dislike the vector solution.

Clang uses std::vector and std::string too liberally, and I don't think that's good enough for LLVM base libraries. The argument that current cpp files already include <vector> is only valid until we start including TargetParser.h in other cpp files that don't. Standard library headers are very nasty indeed.

However, as you said, any other place will be worse than here... except the future TargetTuple class that doesn't exist yet. The new Tuple is like a Triple on steroids, that knows everything about the target, not just CPU and ARCH, but FPU and extensions, OS and vendor with the necessary and unambiguous context.

For now, I guess we can just leave it here and add a big FIXME on "include <vector>", to remove it as soon as features move to Tuple, and the method itself to move this to Tuple once that's created.

Adding the FIXMEs, removing the spurious brackets and moving the enums to the headers, LGTM. Thanks!

cheers,
--renato

lib/Target/ARM/AsmParser/ARMAsmParser.cpp
9199 ↗(On Diff #27102)

no need for brackets any more here either.

This revision is now accepted and ready to land.Jun 4 2015, 10:04 AM
jroelofs added inline comments.
lib/Support/TargetParser.cpp
35 ↗(On Diff #27102)

it would be much better to encode these comments like this:

FR_None,   //< No Restriction
FR_D16,    //< Only 16 D registers
FR_SP_D16, //< Only single precision instructions, with 16 D registers
48 ↗(On Diff #27102)

and this one like this:

unsigned FPUVersion; //< corresponds directly to the FP arch version number
This revision was automatically updated to reflect the committed changes.