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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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
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.