This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Enable objdump to construct a useful triple for ARM
ClosedPublic

Authored by samparker on Jan 4 2017, 1:44 AM.

Details

Summary

Following from D28227, the ARMAttributeParser now stores the attributes in a map and can be constructed without a printer. ELFObjectFile can now use the cpu attribute to build up a triple, which can then be used by llvm-objdump and results in the user only needing to specify either 'arm' or 'thumb' as the triple. The next patch will extend this further and add subtarget features, in a similar manner to MIPS.

Diff Detail

Repository
rL LLVM

Event Timeline

samparker updated this revision to Diff 83018.Jan 4 2017, 1:44 AM
samparker retitled this revision from to [ARM] Enable objdump to construct a useful triple for ARM.
samparker updated this object.
samparker added a subscriber: llvm-commits.

I'm expecting some tests to make sure that the new attributes are correctly parsed, printed, and that objects without a triple, but with attributes, can pick the right triple from them.

include/llvm/Support/ARMAttributeParser.h
24 ↗(On Diff #83018)

Why map to a pair if you only use the unsigned value?

lib/Object/ELFObjectFile.cpp
125 ↗(On Diff #83018)

I don't like this function being here. It should be encoded from some table gen description or from the target parser, but that shouldn't be done in this commit.

Please add a FIXME to that effect.

140 ↗(On Diff #83018)

This list doesn't have all possible values documented in the ABI.

Hi Renato,

Thanks for the review. For attribute printing and parsing, I was hoping that the current readobj tests would be sufficient since hopefully I have not changed the functionality of that tool. Also, how would you suggest building a triple from just the build attributes? Currently I use the existing triple to only decide whether it should be an arm or thumb triple and I don't believe build attributes can help here. Unless you mean for thumb only targets?

Cheers,

include/llvm/Support/ARMAttributeParser.h
24 ↗(On Diff #83018)

This was so that if someone else wanted the string value from a string attribute, it would be available.

lib/Object/ELFObjectFile.cpp
140 ↗(On Diff #83018)

good catch, i will add them.

Thanks for the review. For attribute printing and parsing, I was hoping that the current readobj tests would be sufficient since hopefully I have not changed the functionality of that tool.

That is true, but now the library lives in LLVM, and just like Clang, we need to test both the tool and the library.

I'd recommend us having unit-tests for this, as we already have them for the Triple and TargetParser. It'd be much easier to test this with unit-tests than ASM files, though we could also have some of the latter, to make sure the whole cycle is complete.

Also, how would you suggest building a triple from just the build attributes? Currently I use the existing triple to only decide whether it should be an arm or thumb triple and I don't believe build attributes can help here. Unless you mean for thumb only targets?

Sorry, I didn't mean to change the triple, but to change the supported features from the build attributes. Thumb/ARM, VFP/NEON and other architectural support need to be restricted if the build attributes mandate so. Those features are set by default from the triple, but if the build attribute overrides, then we have to error out when parsing the assembly file.

cheer,
--renato

include/llvm/Support/ARMAttributeParser.h
24 ↗(On Diff #83018)

When that is needed, we add. No need to hold huge strings (compared to an int) in the map today.

Ah ok, then I will get some unit tests together. For the build attributes overriding triple defaults, could I not just use remove the feature from the SubtargetFeatures in the following patch?

cheers,
sam

samparker updated this revision to Diff 84110.Jan 12 2017, 5:11 AM

I've added the missing cpu architectures, added a few enums to the build attributes and added a unit test for the attribute parser that covers the "aeabi" attributes.

rengolin accepted this revision.Jan 12 2017, 11:42 AM
rengolin added a reviewer: rengolin.

Nice! Thanks! LGTM.

This revision is now accepted and ready to land.Jan 12 2017, 11:42 AM
This revision was automatically updated to reflect the committed changes.