This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Compute a target feature which corresponds to the ARM version.
ClosedPublic

Authored by efriedma on Apr 3 2018, 6:21 PM.

Details

Summary

Currently, the interaction between the triple, the CPU, and the supported features is a mess: the driver edits the triple to indicate the supported architecture version, and the LLVM backend uses this to figure out what instructions are legal. This makes it difficult to understand what's happening, and makes it impossible to LTO together two modules with different computed architectures.

Instead of relying on triple rewriting to get the correct target features, we should add the right target features explicitly.

Diff Detail

Repository
rC Clang

Event Timeline

efriedma created this revision.Apr 3 2018, 6:21 PM
SjoerdMeijer accepted this revision.Apr 4 2018, 12:23 AM

Agree, this is a real mess.
This change looks good to me.

Was only wondering, we are only testing target features +v7 and +v8, but do we now also need to check the others, like +v8.1a, +v8.2a, etc.?

This revision is now accepted and ready to land.Apr 4 2018, 12:23 AM

Thanks for looking into this Eli! Adding the architecture version as target feature looks good to me.

lib/Basic/Targets/ARM.cpp
342

Would it make sense to add this to llvm/Support/TargetParser? We already have AArch64::getArchFeatures there and it looks like it does something similar for AArch64.

llvm/Support/ARMTargetParser.def defines the different architectures + extensions and it seems like it should possible to extend it slightly to handle the architecture features.

fhahn added a comment.Apr 4 2018, 2:16 AM

We also add thumb-mode to the target features, for a similar reason, allowing mixed Thumb/Arm codegen with LTO.

efriedma updated this revision to Diff 141078.Apr 4 2018, 3:55 PM

Fixed to use the arch name as the "feature"; otherwise, we miss relevant features in certain cases. This is taken from ARMAsmParser::parseDirectiveArch.

fhahn added inline comments.Apr 5 2018, 7:53 AM
lib/Basic/Targets/ARM.cpp
409

Is there a reason we re-compute the Arch from the triple here, but use the member variable ArchKind above? Shouldn't they both be the same ?

efriedma added inline comments.Apr 5 2018, 12:50 PM
lib/Basic/Targets/ARM.cpp
409

ArchKind is based on the triple and the CPU, where "Arch" is just based on the triple, so "Arch" was returning the wrong thing in some cases.

Actually, looking a bit more, we should probably be calling parseCPUArch here to get the right behavior, rather than depending on the member. (It doesn't usually matter, but it could make a difference for code using "target" attribute, I guess. That feature needs a lot more work, though.)

efriedma updated this revision to Diff 141237.Apr 5 2018, 4:02 PM

Compute arch used passed-in CPU, not the global arch.

rengolin added a subscriber: joerg.Apr 6 2018, 5:04 AM

LTO is something I never considered before in the context of the target parser, but I understand the issues are similar to what the build attributes were trying to solve, so adding more info shouldn't make it worse.

However, this will probably break obscure cases that normally only @compnerd and @joerg pick up. I, personally, don't have any opinion on how much this makes sense or not. :/

I'm with @fhahn that we should make ARM and AArch64 as common as possible, not only in code for the target parser but also in behaviour for the front-end, mostly because ARMv8 spans both and we want to avoid surprises.

joerg added a comment.Apr 6 2018, 5:25 AM

Can you make sure that we handle the older ARM versions correctly as well, i.e. v4, v5 and v6? I take it we still have test cases for the arm <-> thumb transition? That's the one part of the triple logic that is really non-trivial.

fhahn accepted this revision.Apr 6 2018, 11:06 AM

Thanks for updating it to use the stuff from TargetParser.

LGTM, with tests for the cases @joerg mentiond.

lib/Basic/Targets/ARM.cpp
409

Thanks that makes sense. getDefaultFPU and getDefaultExtensions are only for "generic" FPUs.

efriedma updated this revision to Diff 141393.Apr 6 2018, 11:38 AM

Add a bunch of tests for various arm arches. Make sure we do something reasonable when we can't figure out any arch at all.

I take it we still have test cases for the arm <-> thumb transition?

"-mthumb" and "-marm" are handled in the driver by rewriting the triple.

This revision was automatically updated to reflect the committed changes.

hey there, I've run into problems with stripping static-libs on arm when using llvm/clang-7. Could you imagine this patch being at fault?

strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R .GCC.command.line -R .note.gnu.gold-version
   lib/libbz2.so.1.0.6
   usr/bin/bzip2recover
   bin/bzip2
   usr/lib/libbz2.a
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11

hey there, I've run into problems with stripping static-libs on arm when using llvm/clang-7. Could you imagine this patch being at fault?

strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R .GCC.command.line -R .note.gnu.gold-version
   lib/libbz2.so.1.0.6
   usr/bin/bzip2recover
   bin/bzip2
   usr/lib/libbz2.a
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11

It doesn't seem that likely to me. The error you are seeing looks like a broken object file, the patch here will affect the type of features a target might use during code generation but it shouldn't affect strip.

I suggest dumping the ELF file with readelf or objdump and look at the section output for section 11. Some sections have a special meaning for the sh_link field (http://www.sco.com/developers/gabi/latest/ch4.sheader.html#sh_link) it looks like strip is complaining that either a section is missing a sh_link field that it should have or it has an invalid index.

It will also be worth seeing if the objects in that library have been processed by any other tool such as objcpy to see if it has introduced an error.