Page MenuHomePhabricator

Fix ARMv4 support
ClosedPublic

Authored by joerg on Aug 22 2017, 3:11 PM.

Details

Summary

ARMv4 doesn't support the "BX" instruction, which has been introduced with ARMv4t. Adjust the call lowering and tail call implementation accordingly.

Further changes are necessary to ensure that presence of the v4t feature is correctly set. Most importantly, the "generic" CPU for thumb-* triples should include ARMv4t, since thumb mode without thumb support would naturally be pointless.

Add a couple of asserts to ensure thumb instructions are not emitted without CPU support.

Diff Detail

Repository
rL LLVM

Event Timeline

joerg created this revision.Aug 22 2017, 3:11 PM
olista01 added inline comments.
lib/Target/ARM/ARMExpandPseudoInsts.cpp
1028 ↗(On Diff #112238)

Could you add a test covering the ARM::TAILJMPr4 case?

fhahn added a subscriber: fhahn.Aug 23 2017, 2:39 AM
fhahn added inline comments.
lib/Target/ARM/ARMAsmPrinter.cpp
1275 ↗(On Diff #112238)

Maybe this assert would be clearer if the assert contained something like && "BX instructions are only available in ARMv4t and later". Applies to all added asserts in this patch

fhahn added a subscriber: rengolin.Aug 23 2017, 3:16 AM

I think this change makes sense. Looping in @rengolin as he may have some more insight

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1912 ↗(On Diff #112238)

I'm not sure if this assertion is really needed here. If the block does not already contain a BX instruction, we exit early.

rengolin added inline comments.Aug 23 2017, 3:30 AM
lib/Target/ARM/ARMAsmPrinter.cpp
1275 ↗(On Diff #112238)

Well, both BX_CALL and BX_RET have Requires<[IsARM, HasV4T]>, which ARMv4 (in ARM.td) doesn't have.

So, either this BX_CALL is being generated manually (and incorrectly), or there's something wrong with the ISel.

I think it's safe to assume that, if a target has BX_CALL, it also has BX_RET, so the assert here should be redundant.

Your princess might be in another castle. :)

PS: It's possible that the introduction of getReturnOpcode() has fixed that already.

t.p.northover accepted this revision.Aug 28 2017, 7:35 AM

I think the issues have been basically resolved here. The assertion can't hurt. LGTM!

This revision is now accepted and ready to land.Aug 28 2017, 7:35 AM
joerg added inline comments.Aug 28 2017, 7:46 AM
lib/Target/ARM/ARMAsmPrinter.cpp
1275 ↗(On Diff #112238)

Yes, it is pretty obvious from the instructions why the assert exists. The main motivation for the asserts is to ensure that all places where we create them by hand also have the feature around. Given that we missed a couple of them, I prefer to be safe here.

lib/Target/ARM/ARMExpandPseudoInsts.cpp
1028 ↗(On Diff #112238)

armv4.ll tests the tail call.

lib/Target/ARM/ARMLoadStoreOptimizer.cpp
1912 ↗(On Diff #112238)

I don't think it hurts either, though. It makes it easier to search for potentially missing places.

This revision was automatically updated to reflect the committed changes.